Skip to content

Redefine 'not' to not emit IL directly #9434

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

cartermp
Copy link
Contributor

@cartermp cartermp commented Jun 12, 2020

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

@forki
Copy link
Contributor

forki commented Jun 12, 2020

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

@cartermp
Copy link
Contributor Author

oh poopy, failures

@cartermp
Copy link
Contributor Author

cartermp commented Jun 12, 2020

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

@abelbraaksma
Copy link
Contributor

abelbraaksma commented Jun 12, 2020

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 cartermp changed the title Redefine 'not' to just use pattern matching Redefine 'not' to not emit IL directly Jun 12, 2020
@cartermp
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

TIHan commented Jun 16, 2020

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

@cartermp
Copy link
Contributor Author

@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
Copy link
Member

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.

@@ -398,7 +398,7 @@ namespace Microsoft.FSharp.Core
let inline unboxPrim<'T>(x:obj) = (# "unbox.any !0" type ('T) x : 'T #)
let inline box (x:'T) = (# "box !0" type ('T) x : obj #)
let inline convPrim<'T1, 'T2>(x: 'T1) : 'T2 = unboxPrim<'T2> (box x)
let inline not (b:bool) = (# "ceq" b false : bool #)
let inline not (b:bool) = if b then false else true
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@abelbraaksma
Copy link
Contributor

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
Copy link
Contributor Author

git is hard, closing

@abelbraaksma
Copy link
Contributor

abelbraaksma commented Jul 19, 2020

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 :).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

not(isNull x) leads to very odd and partially unreachable IL code that performace 5x slower than redefining not yourself
5 participants