remove bounds checks for some more unsigned comparisons #43568
Conversation
Dotnet-GitSync-Bot
added
the
area-CodeGen-coreclr
|
PTAL @dotnet/jit-contrib |
|
/azp run runtime-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Looks good in general, I like the testing, thanks @nathan-moore!
A few nits and questions.
Assuming we don't want to keep it open to track arr.Length >= (uint)1, this fixes #11623
Could you please clarify this? Is this case optimized now? Is not it the same case as "if (destination.Length >= 5)" that is already marked as eliminated in #11623 (comment)
|
// Auto-generated message 69e114c which was merged 12/7 removed the intermediate src/coreclr/src/ folder. This PR needs to be updated as it touches files in that directory which causes conflicts. To update your commits you can use this bash script: https://gist.github.com/ViktorHofer/6d24f62abdcddb518b4966ead5ef3783. Feel free to use the comment section of the gist to improve the script for others. |
There was a problem hiding this comment.
@nathan-moore do you have time to finish this PR now? Could you please fix the typos/add requested comments and answer a few questions that were left?
Then I will review it again and merge.
JulieLeeMSFT
added
needs author feedback
blocked
Ping @nathan-moore to answer to sandreenko's question. |
nathan-moore
force-pushed
the
GreaterEqualRemoval
branch
from
f1177e4 to
39ab103
Compare
|
Sorry about taking so long to get back to this.
This isn't something that is optimized currently. If my memory serves right, internally a cast is inserted for the array length, and then value numbering puts a different vn on the cast. From a bounds check perspective, I don't see anyone writing code like this, but we can keep the issue open to track it. Note that similar patterns work This pattern has also become more popular since I last touched this: Total bytes of base: 31548082
Total bytes of diff: 31547910
Total bytes of delta: -172 (-0.001% of base)
diff is an improvement.
Top file improvements (bytes):
-128 : System.Private.CoreLib.dasm (-0.004% of base)
-44 : System.Net.Http.dasm (-0.008% of base)
2 total files with Code Size differences (2 improved, 0 regressed), 267 unchanged.
Top method regressions (bytes):
2 (0.056% of base) : System.Private.CoreLib.dasm - DateTimeFormatInfo:CreateTokenHashTable():ref:this
Top method improvements (bytes):
-130 (-7.545% of base) : System.Private.CoreLib.dasm - DateTimeParse:ParseFormatO(ReadOnlySpan`1,byref):bool
-20 (-9.756% of base) : System.Net.Http.dasm - HPackEncoder:EncodeLiteralHeaderFieldWithoutIndexingNewName(String,ReadOnlySpan`1,String,Encoding,Span`1,byref):bool
-12 (-7.643% of base) : System.Net.Http.dasm - HPackEncoder:EncodeLiteralHeaderFieldWithoutIndexing(int,String,Span`1,byref):bool
-12 (-9.917% of base) : System.Net.Http.dasm - HPackEncoder:EncodeLiteralHeaderFieldWithoutIndexingNewName(String,Span`1,byref):bool
Top method regressions (percentages):
2 (0.056% of base) : System.Private.CoreLib.dasm - DateTimeFormatInfo:CreateTokenHashTable():ref:this
Top method improvements (percentages):
-12 (-9.917% of base) : System.Net.Http.dasm - HPackEncoder:EncodeLiteralHeaderFieldWithoutIndexingNewName(String,Span`1,byref):bool
-20 (-9.756% of base) : System.Net.Http.dasm - HPackEncoder:EncodeLiteralHeaderFieldWithoutIndexingNewName(String,ReadOnlySpan`1,String,Encoding,Span`1,byref):bool
-12 (-7.643% of base) : System.Net.Http.dasm - HPackEncoder:EncodeLiteralHeaderFieldWithoutIndexing(int,String,Span`1,byref):bool
-130 (-7.545% of base) : System.Private.CoreLib.dasm - DateTimeParse:ParseFormatO(ReadOnlySpan`1,byref):bool
Also, I noticed that we're still lacking test coverage for when the bounds check removal is on the next edge. I can add that tuesday. |
JulieLeeMSFT
removed
blocked
There was a problem hiding this comment.
LGTM, thanks @nathan-moore . I like this version that is much simpler than the previous one.
Not sure I understand, could you please explain? |
|
/azp run runtime-coreclr jitstress |
|
Azure Pipelines successfully started running 1 pipeline(s). |
It's not a big deal. I just have a feeling that someone is going to see <= suddenly get treated as < and get confused. |
ghost
locked as resolved and limited conversation to collaborators
Remove bounds checks for unsigned constant comparisons against constants of the form
(uint)arr.Length >= 1(uint)arr.Length >= (uint)1Of all the different ways of permutating uint casts, the only one we don't remove bound checks is for forms like arr.Length >= (uint)1, due to the vn changing on the array length due to the inserted cast. I haven't looked, but I don't think this particular pattern is common.
Only diff for this change is in corelib:
Total bytes of delta: -128 (-0.004% of base) diff is an improvement. Top file improvements (bytes): -128 : System.Private.CoreLib.dasm (-0.004% of base) 1 total files with Code Size differences (1 improved, 0 regressed), 0 unchanged. Top method regressions (bytes): 2 (0.056% of base) : System.Private.CoreLib.dasm - DateTimeFormatInfo:CreateTokenHashTable():ref:this (2 methods) Top method improvements (bytes): -130 (-7.501% of base) : System.Private.CoreLib.dasm - DateTimeParse:ParseFormatO(ReadOnlySpan`1,byref):bool Top method regressions (percentages): 2 (0.056% of base) : System.Private.CoreLib.dasm - DateTimeFormatInfo:CreateTokenHashTable():ref:this (2 methods) Top method improvements (percentages): -130 (-7.501% of base) : System.Private.CoreLib.dasm - DateTimeParse:ParseFormatO(ReadOnlySpan`1,byref):boolThe regression is that we run out of table space generating an unused assertion, and thus don't eliminate a null check.
Assuming we don't want to keep it open to track arr.Length >= (uint)1, this fixes #11623
cc @briansull