Remove inline IL for public 'not' function

[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:

module Op =
    let inline not' x = if x then false else true
    let inline not'' x = match x with true -> false | false -> true


[<MemoryDiagnoser>]
type NotBench() =
    let s = "hello"

    [<Benchmark(Baseline=true)>]
    member _.Not() = 
        let mutable b = false
        for i=0 to 1 do b <- not (isNull s)
        b

    [<Benchmark>]
    member _.NotWithIf() = 
        let mutable b = false
        for i=0 to 1 do b <- Op.not' (isNull s)
        b

    [<Benchmark>]
    member _.NotWithMatch() = 
        let mutable b = false
        for i=0 to 1 do 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	-	-	-	-

[abelbraaksma]

For my understanding, this looks like a giant change for one line of code, does this have something to do with specific IL tests, that are generated from time to time to prevent accidental IL regressions?

[cartermp]

Ugh, looks like getting these ExprTests to pass locally doesn't mean anything in a CI environment. These tests are so hard to understand

@abelbraaksma Yes, the huge changes are about IL that was generated many years ago and how it needs to change given that not has been changed to emit better IL. It's frustrating to deal with, unfortunately

[cartermp]

These FSharpQA tests are impossible to update or get to pass. I don't know how to progress with this PR.

[abelbraaksma]

Ouch, sounds painful. I doubt I can help, but since it was my report to begin with, I can give it a shot (likely to strand where you stranded, but then at least we're both frustrated :p)

[abelbraaksma]

From a cursory look at the tests, it appears it's the same set that I ran into for updating string. I start to understand why nobody dares attacking these basic functions: too much effort getting the tests updated.

[cartermp]

Given some other changes to these files, where similar seemingly unrelated changes had to go in just to get the tests to pass, I'm certain now that whatever the tool FSharpQA tests use to generate those baseline tools is very much out of date and shouldn't be considered to greatly.

@KevinRansom sorry to call upon you here, but I have absolutely no clue how to get this FSharpQA test to pass.

[KevinRansom]

@cartermp, will take a look. I remember looking earlier and wondering if it was a real issue, which worried me cos I like the fix.

[abelbraaksma]

Oops, I forgot that you continued from #9434 here.

I've meanwhile taken a slightly different approach which leaves the not optimizations used for generic comparisons in place as-is (they weren't wrong), and has the public not function compile as done here. Sorry for the hijack ;).

@cartermp This new PR is green now: #9715, maybe close this in favor of that one?

About this:

Ugh, looks like getting these ExprTests to pass locally doesn't mean anything in a CI environment. These tests are so hard to understand

Some of those problems are now alleviated with #9622 (mainly by giving per-line feedback and a more predictable compile-run-cleanup cycle). But some of the failures you see in CI but not locally are caused by the fact that CI runs some tests with old FSharp.Core.dll (meaning: change the IL generated by FSharp.Core, update the tests, this will then fail when the same test is run against the 4.5 version of the lib). I ran into something similar here (#9549) where I ultimately decided to disable some tests based on what FSharp.Core version is loaded. Once we go over this again with @vzarytovskii, I hope to come to a cleaner way of writing / running these.

[cartermp]

Closing in favor of #9715