New attempt to fix 'not' function

[abelbraaksma]

Fixes #9433
Depends on #9622 (I really like to merge that one, as more and more are depending on it, making reviews harder, @KevinRansom could you check that one please?)

This resurrects #9434 by trying a different approach:

• I noticed that many optimizations depend on inline IL (see mkAssemblyCodeValueInfo), by leaving this behavior in place for comparison and logic functions, this makes less changes to baseline tests and FCS tests (like ExprTests)
• This moves the public function down in prim-types.fs, so that the "basic inlined operation" comes into scope, and changes it to use normal syntax.

The main benefit is: better optimized IL in user code, esp wrt not(isNull x), which is currently broken, see linked issue.

Relevant commit after #9622: f2a9e2c

Let's see what tests are still complaining (impossible to run all locally).

[abelbraaksma]

Failing build, but build runner shows 😆:

[abelbraaksma]

Strange stuff, a test clearly failed but CI doesn't consider it a fail (see above), but reports the run as failed 🤔

[abelbraaksma]

This fixes this bsl test (note that I don't seem to be able to run these tests locally with build -testFSharpQA, a bunch always fail and after some time it just hangs (no CPU, can see some perl jobs in task mgr) after this:

Optimizations\Inlining (StructUnion01.fs) -- passed

Hmm, deja vu, I think I had issues with these tests not so long ago.

[abelbraaksma]

This is ready, @cartermp can you review? Though it would be nice if #9622 gets merged first, to limit the diff.

[cartermp]

#9622 is now merged. For some reason GitHub still shows a large diff that includes its changes. Would it be possible to rebase this atop the latest master to make it a cleaner diff?

[abelbraaksma]

Would it be possible to rebase this atop the latest master to make it a cleaner diff?

@cartermp, done (well, without the rebase, it appeared that just a merge from 'master' was fine). The "changed list" shows only the relevant changes now.

I'll do the same for the other PRs that depend on #9622.

[cartermp]

Thanks, looks great!

[abelbraaksma]

With Kevin OOO, can I kindly ask other jedis to do the 2nd review? @TIHan, @dsyme perhaps?

[abelbraaksma]

Never mind, looks like this one only needed one review? Great 👍

[cartermp]

Yeah, this is implemented exactly as suggested and the changes look good. Thanks!

[KevinRansom]

@abelbraaksma ,
the reason for the issue is an underlying problem in the "ceq" code generation. Looking through the compiler code "ceq" is used quite a lot including with bool arguments, that means that there are other ways of creating the issue highlighted in the bug.

So I think the correct approach is to understand the underline ceq issue and try to fix that. I understand that this fix correctly resolves this symptom, and this is a very commonly used function, however, I think we need a better understanding of how to fix ceq before making these kind of band aid fixes.

I hope that makes sense

Kevin

[cartermp]

@KevinRansom I definitely disagree here. We should not be in the business of emitting IL directly anymore unless it is absolutely necessary. This is the right approach for FSharp.Core.

[KevinRansom]

@cartermp ,
We are a compiler, emitting il is what we do. ceq is used a fair amount in our code generation pipeline. Of course if we want to eliminate "ceq" support from the compiler, then a PR doing that would be welcome.

Kevin

[cartermp]

We should have a chat. I don't think you are right. This is not the compiler, this is a public API function in the F# core library for flipping a boolean expression.

The fact that raw IL emission is was done historically is just history and not at all related to emitting IL as a part of compilation. We should not be in the business of this stuff unless it's necessary for bootstrapping purposes. This function is definitely not involved in any kind of bootstrapping.

[cartermp]

Please refer to the discussion here for what led to this change: #9433

[KevinRansom]

These types of PR's are always the most difficult for me. Because the fix itself is a no-brainer ... sure the new one line is easier to parse and clearly better than the old one line.

However, there are plenty of other places in the compiler that still generate the equivalent of the old one line. They don't benefit from the change, and this PR does not even attempt to fix them. I would prefer to understand and perhaps fix the actual problem rather than just avoid it in the places developers notice it.

[abelbraaksma]

@KevinRansom, @cartermp, When working with this fix I noticed some other cases where incorrect boolean optimizations are done. I'm still analyzing that, but this fix produces better IL for the majority of cases.

The optimizer is split in optimizing inline IL and normal code. When it is mixed, esp wrt booleans, it goes wrong and dead IL code is created. What's worse, when combined with null checks, the code didn't eliminate the not, but created code that was many times slower.

This simple fix takes care of all that. Yes, there are now other cases, but the way this fix was done ensures that the new cases are very limited, and that existing optimizations with comparison operators continue to work.

I intend to investigate further for optimizations of boolean expressions, esp boolean negation. But that should go in its own issue. I'll report that separately, as it's a much wider issue than this one (and may end up being wip for some time).

[abelbraaksma]

They don't benefit from the change, and this PR does not even attempt to fix them

@KevinRansom No, this is misunderstanding what I'm doing here, sorry for not being clearer. The code using the old version gets optimized properly. The wrong assumption in the previous PR was that just changing not would lead to optimized code everywhere, on all paths. It doesn't.

• the inline IL version leads to optimized code when paired with other inline IL function or operator.
• the normal function (the new not) leads to optimized IL in most cases when paired with another normal function (like isNull). I.e. br.false becomes br.true and vv.
• this doesn't fix the problem of the optimizer when encountering a mix of inline IL and normal code (rare, but possible), nor does it fix certain rare cases where negation always leads to bad IL (see the single bsl file that failed after this change).

This difference in behavior can be seen in the previous PRs by @cartermp, which show in the diff of the bsl files the larger, and partially dead IL code being generated if we were to fix not for all code paths.

That needs to be fixed, sure, but that won't be trivial and I'd very much like to do that in a separate issue.

However, there are plenty of other places in the compiler that still generate the equivalent of the old one line.

To sum up: this is deliberate, otherwise the cure would be worse than the illness, I don't want that on my conscience ;)

[KevinRansom]

@abelbraaksma the baseline that changed was built non-optimized, when that test case is built optimized no change in il happens.

[KevinRansom]

As I said before, this PR is tricky. I am prepared to approve it, since there is a clear benefit, however, the underlying issue remains. We should address that too at some point.

[abelbraaksma]

the baseline that changed was built non-optimized, when that test case is built optimized no change in il happens

This came as a big surprise to me and basically set me on a tour to review my earlier assessments.

After quite a bit experimenting I finally got a nice setup to compare before & after IL for different fsc arguments. I now come to the same conclusion as you, except that I need to verify some more optimized code.

It is indeed as you say that many lines of somewhat weirdly looking, and partially dead IL in debug builds end up being a single IL instruction as it should (plus ret) for the offending case.

This makes one wonder, though, why we compare so many IL cases for debug builds, where the more interesting case is clearly the release build here.

@KevinRansom, I apologize for coming so strongly to the wrong conclusion, I misunderstood how these bsl files were generated.

Some of the code I see in the debug build that's clearly redundant, may be part of the cause that you need to hit "next step" multiple times in certain cases when debugging, where there isn't such extra step in the code (not even if you desugar things). That'll be a research for another day.

I'll create the relevant issue for improving the boolean negation further, but that'll be a much larger task given that it changes all the IL for the negated comparison operators.

Thanks for reviewing this, @KevinRansom.

[KevinRansom]

@abelbraaksma , no problem mate, and thanks for this pr.

[abelbraaksma]

Follow-up issue: #9753.