- Notifications
You must be signed in to change notification settings - Fork5.1k
JIT: Range Checks for "(uint)i > cns" pattern#62864
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ghost commentedDec 15, 2021
Tagging subscribers to this area:@JulieLeeMSFT Issue DetailsExample:staticReadOnlySpan<byte>MySpan=>newbyte[]{1,2,3,4,5};intGetElement(intindex){if((uint)index<MySpan.Length)returnMySpan[index];return0;} Codegen diff:; Method Program:GetElement(int):int:thisG_M56827_IG01:- sub rsp, 40G_M56827_IG02:- mov eax, edx- cmp rax, 5- jge SHORT G_M56827_IG05+ cmp edx, 5+ jae SHORT G_M56827_IG05G_M56827_IG03:- cmp edx, 5- jae SHORT G_M56827_IG07 mov eax, edx mov rdx, 0xD1FFAB1E movzx rax, byte ptr [rax+rdx]G_M56827_IG04:- add rsp, 40 ret G_M56827_IG05: xor eax, eaxG_M56827_IG06:- add rsp, 40 ret-G_M56827_IG07:- call CORINFO_HELP_RNGCHKFAIL- int3-; Total bytes of code: 51+; Total bytes of code: 25
|
EgorBo commentedDec 15, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
As a side bonus it removes bound checks for this: staticintFoo(int[]array,inti){if((uint)i<array.Length)returnarray[i];return0;} Previously itneeded |
EgorBo commentedDec 17, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
For all of these variations bound checks are now eliminated (none of them are eliminated in.NET 6.0): publicclassTests{staticReadOnlySpan<byte>MySpan=>newbyte[]{1,2,3,4,5};publicstaticintTest1(intindex){if((uint)index<MySpan.Length)returnMySpan[index];return0;}publicstaticintTest2(intindex){if(MySpan.Length>(uint)index)returnMySpan[index];return0;}publicstaticintTest3(intindex){if(MySpan.Length<=(uint)index)return0;returnMySpan[index];}publicstaticintTest4(intindex){if((uint)index>=MySpan.Length)return0;returnMySpan[index];}publicstaticintTest5(intindex){if(index<0||index>=MySpan.Length)return0;returnMySpan[index];}publicstaticintTest6(intindex){if(index>=0&&index<MySpan.Length)returnMySpan[index];return0;}publicstaticintTest7(intindex){if(index<0)thrownewException();if(index>=MySpan.Length)thrownewArgumentException();returnMySpan[index];}publicstaticintTest8(intindex){if((uint)index<2)// 2 is less than MySpan.LengthreturnMySpan[index];return0;}publicstaticintTest9(intindex){if(index<2||index>4)// e.g. we restrict index to be in [2..3] range despite the fact MySpan is fine with [0..4]thrownewArgumentException();returnMySpan[index];}} Unrelated pattern that got fixed too: intfoo(uinti,int[]array){if(i<array.Length)returnarray[i];return0;} if index is defined as unsigned - we don't need any casts to uint. |
e87d3b2
to58f4f68
Compare/azp run Fuzzlyn, Antigen |
Azure Pipelines successfully started running 2 pipeline(s). |
EgorBo commentedDec 20, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@dotnet/jit-contrib@SingleAccretion PTAL (Antigen failures are not related) I generated a lot of test cases locally (~100k lines of code) e.g.https://gist.github.com/EgorBo/1ba16f0d9b1b7d87044c7ed4d14db117 to validate there are no surprises. But I don't think I should add them to the repo as they are too big/slow to run and don't add much value. |
EgorBo commentedDec 23, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
…/runtime-1 into bound-checks-constant-len
@dotnet/jit-contrib PTAL |
…hecks-constant-len# Conflicts:#src/coreclr/jit/compiler.h#src/coreclr/jit/morph.cpp
…hecks-constant-len
…hecks-constant-len
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
EgorBo commentedFeb 23, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@jakobbotsch can you take a look again? New diffs:https://dev.azure.com/dnceng/public/_build/results?buildId=1621068&view=ms.vss-build-web.run-extensions-tab I inspected a couple of minor regressions and they seem to be caused by CSE/RA |
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.
LGTM, just had one question.
cc@SingleAccretion in case you want to give this a lookover as well.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com>
…hecks-constant-len
@SingleAccretion thanks for the feedback!! addressed almost all of it except BashToNop - I'm leaving it to whoever will refactor it to optNarrowTree. Failures are not related:#65818 |
Uh oh!
There was an error while loading.Please reload this page.
Closes#13464
Closes#1343
Example:
Codegen diff:
(uint)
cast is still needed for index because it can be negative. Alternative patternindex >= 0 && index < MySpan.Length
also works here.For all of these variations bound checks are now eliminated (none of them are eliminated in.NET 6.0):
Unrelated pattern that got fixed too:
if index is defined as unsigned - we don't need any casts to uint.