Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Conversation

@mikedn
Copy link

@mikedn mikedn commented Oct 4, 2017

This optimization is not valid for unsigned LT/LE/GT/GE relops. Using the Carry flag this way indicates that the operation overflowed, not that the result is less than 0, that's impossible for unsigned integers.

Fixes #14321

@mikedn
Copy link
Author

mikedn commented Oct 4, 2017

jit-diff summary:

Total bytes of diff: 345 (0.00% of base)
    diff is a regression.
Total byte diff includes 0 bytes from reconciling methods
        Base had    0 unique methods,        0 unique bytes
        Diff had    0 unique methods,        0 unique bytes
Top file regressions by size (bytes):
         133 : System.Private.CoreLib.dasm (0.00% of base)
         116 : System.Text.RegularExpressions.dasm (0.12% of base)
          27 : Microsoft.CSharp.dasm (0.01% of base)
          13 : Microsoft.CodeAnalysis.CSharp.dasm (0.00% of base)
          12 : System.Collections.Concurrent.dasm (0.02% of base)
14 total files with size differences (0 improved, 14 regressed), 65 unchanged.
Top method regessions by size (bytes):
          38 : System.Private.CoreLib.dasm - GenericArraySortHelper`1:PickPivotAndPartition(ref,int,int):int (18 methods)
          22 : System.Text.RegularExpressions.dasm - RegexParser:ScanCharClass(bool,bool):ref:this
          20 : System.Private.CoreLib.dasm - GenericArraySortHelper`1:DownHeap(ref,int,int,int) (18 methods)
          18 : System.Text.RegularExpressions.dasm - RegexParser:ScanGroupOpen():ref:this
          15 : System.Private.CoreLib.dasm - GenericArraySortHelper`1:SwapIfGreaterWithItems(ref,int,int) (18 methods)
Top method improvements by size (bytes):
          -1 : System.Private.CoreLib.dasm - Task:FinishSlow(bool):this
          -1 : System.Private.CoreLib.dasm - Task:ProcessChildCompletion(ref):this
          -1 : System.Private.CoreLib.dasm - Task:WaitAllBlockingCore(ref,int,struct):bool
          -1 : System.Private.CoreLib.dasm - SetOnCountdownMres:Invoke(ref):this
          -1 : System.Private.CoreLib.dasm - WhenAllPromise:Invoke(ref):this
88 total methods with size differences (8 improved, 80 regressed), 66464 unchanged.

So we're back where we were before #14027.

This optimization is not valid for unsigned LT/LE/GT/GE relops. Using the Carry flag this way indicates that the operation overflowed, not that the result is less than 0, that's impossible for unsigned integers.

This is also not valid for signed LT/LE/GT/GE relops due to integer overflow.
@BruceForstall
Copy link

@dotnet/jit-contrib

@jkotas
Copy link
Member

jkotas commented Oct 5, 2017

@dotnet/jit-contrib I am merging this because of it is fixing corefx test break. If you have any feedback it can be addressed in follow up PR.

@jkotas jkotas merged commit d0baa73 into dotnet:master Oct 5, 2017
@mikedn mikedn deleted the cmp-uns-flags branch December 16, 2017 09:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants