-
Notifications
You must be signed in to change notification settings - Fork 825
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
Conversation
What happens if you just use if/then? Pattern matching looks really ugly on bool |
oh poopy, failures |
if/then emits the same IL (and same benchmark results), I can change that |
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 |
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. |
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 |
@cartermp Because the IL output is more complex, we should think about ways to reduce it, even if the runtime does folding. |
@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===
Could there be a problem in the tool used to generate baselines? |
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. |
src/fsharp/FSharp.Core/prim-types.fs
Outdated
@@ -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 |
There was a problem hiding this comment.
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
Strangely, if you look at the function's definition in ilspy, both are equal. Only when used with a small function that compares to (though I admit I don't understand the issue with the baselines, perhaps a test expects the old IL output?) |
git is hard, closing |
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 :). |
Fixes #9433
The current implementation of
not
gets funky when you pass in a call toisNull
for a reference type. Probably other quirks too. The following benchmark shows the effect:I get these timings against .NET 5:
And these for net48: