Redefine 'not' to not emit IL directly

[cartermp]

Fixes#9433

The current implementation of not gets funky when you pass in a call to isNull for a reference type. Probably other quirks too. The following benchmark shows the effect:

moduleOp =let inlinenot' x =if x thenfalseelsetruelet inlinenot'' x =match x withtrue->false|false->true[<MemoryDiagnoser>]typeNotBench()=lets="hello"[<Benchmark(Baseline=true)>]member_.Not()=let mutableb=falsefor i=0to1do b <-not(isNull s)
b
[<Benchmark>]member_.NotWithIf()=let mutableb=falsefor i=0to1do b <- Op.not' (isNull s)
b
[<Benchmark>]member_.NotWithMatch()=let mutableb=falsefor i=0to1do b <- Op.not'' (isNull s)
b

I get these timings against .NET 5:

Method	Mean	Error	StdDev	Ratio	Gen 0	Gen 1	Gen 2	Allocated
Not	1.859 ns	0.0224 ns	0.0187 ns	1.00	-	-	-	-
NotWithIf	1.277 ns	0.0178 ns	0.0139 ns	0.69	-	-	-	-
NotWithMatch	1.277 ns	0.0188 ns	0.0157 ns	0.69	-	-	-	-

And these for net48:

Method	Mean	Error	StdDev	Ratio	RatioSD	Gen 0	Gen 1	Gen 2	Allocated
Not	1.909 ns	0.0448 ns	0.0419 ns	1.00	0.00	-	-	-	-
NotWithIf	1.295 ns	0.0357 ns	0.0334 ns	0.68	0.03	-	-	-	-
NotWithMatch	1.292 ns	0.0126 ns	0.0118 ns	0.68	0.02	-	-	-	-

[forki]

What happens if you just use if/then? Pattern matching looks really ugly on bool

[cartermp]

oh poopy, failures

[cartermp]

if/then emits the same IL (and same benchmark results), I can change that

[abelbraaksma]

Hey thanks, that was quick! I thought, since the inline assembly is the same, that something went wrong in the optimizer. But this definitely fixes the general case.

Btw, I've meanwhile found that the issue happens with other functions too, but they have to be small, and have to involve null. Tx for the quick fix!

[cartermp]

So running the baseline update tool updates a lot more than just what was failing, and some of what it updates are more than just comments. I'm also curious why some of the IL has more fields than before.

[cartermp]

I think I might just move these tests to the new test suite because it's horrible trying to update anything to do with fsharpqa baselines

[TIHan]

@cartermp Because the IL output is more complex, we should think about ways to reduce it, even if the runtime does folding.

[cartermp]

@TIHan I'm not sure what's up. I don't see that same IL in sharplab's IL viewer, though: https://sharplab.io/#v2:DYLgZgzgNALiCWwA+wCmMAE8B2weowFsBPAOQHtMAKADxAwCNzzgBKDAXizAxoxgAWqbBjABDYBAKpJBGACcArqiA===

.class public auto ansi abstract sealed _
extends [mscorlib]System.Object
{
.custom instance void [FSharp.Core]Microsoft.FSharp.Core.CompilationMappingAttribute::.ctor(valuetype [FSharp.Core]Microsoft.FSharp.Core.SourceConstructFlags) = (
01 00 07 00 00 00 00 00
)
// Methods
.method public static valuetype [System.Private.CoreLib]System.Boolean myNot (
valuetype [System.Private.CoreLib]System.Boolean x
) cil managed {
// Method begins at RVA 0x2050
// Code size 5 (0x5)
.maxstack 8
IL_0000: ldarg.0
IL_0001: ldc.i4.0
IL_0002: ceq
IL_0004: ret
} // end of method _::myNot
} // end of class _

Could there be a problem in the tool used to generate baselines?

[baronfel]

Sharplab also depends on FCS for this, and if they are behind or there are codegen updates that haven't been released that could be causing their output to be out of sync with what you expect.

[cartermp]

This change is probably the source of my woes w.r.t IL baselines, I wonder why though

[abelbraaksma]

Strangely, if you look at the function's definition in ilspy, both are equal. Only when used with a small function that compares to null, the change is apparent in the generated IL. But that was precisely why we did this change.

(though I admit I don't understand the issue with the baselines, perhaps a test expects the old IL output?)

[cartermp]

git is hard, closing

[abelbraaksma]

Follow-up: I've made a PR with a new attempt in #9715, which uses a slightly different approach than this one, which leads to less changes in the baseline tests (actually, only one). It appears to work now :).