-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
JIT: Range Checks for "(uint)i > cns" pattern #62864
Conversation
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsExample:static ReadOnlySpan<byte> MySpan => new byte[] { 1, 2, 3, 4, 5 };
int GetElement(int index)
{
if ((uint)index < MySpan.Length)
return MySpan[index];
return 0;
} Codegen diff:; Method Program:GetElement(int):int:this
G_M56827_IG01:
- sub rsp, 40
G_M56827_IG02:
- mov eax, edx
- cmp rax, 5
- jge SHORT G_M56827_IG05
+ cmp edx, 5
+ jae SHORT G_M56827_IG05
G_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, eax
G_M56827_IG06:
- add rsp, 40
ret
-G_M56827_IG07:
- call CORINFO_HELP_RNGCHKFAIL
- int3
-; Total bytes of code: 51
+; Total bytes of code: 25
|
As a side bonus it removes bound checks for this: static int Foo(int[] array, int i)
{
if ((uint)i < array.Length)
return array[i];
return 0;
} Previously it needed |
For all of these variations bound checks are now eliminated (none of them are eliminated in .NET 6.0): public class Tests
{
static ReadOnlySpan<byte> MySpan => new byte[] { 1, 2, 3, 4, 5 };
public static int Test1(int index)
{
if ((uint)index < MySpan.Length)
return MySpan[index];
return 0;
}
public static int Test2(int index)
{
if (MySpan.Length > (uint)index)
return MySpan[index];
return 0;
}
public static int Test3(int index)
{
if (MySpan.Length <= (uint)index)
return 0;
return MySpan[index];
}
public static int Test4(int index)
{
if ((uint)index >= MySpan.Length)
return 0;
return MySpan[index];
}
public static int Test5(int index)
{
if (index < 0 || index >= MySpan.Length)
return 0;
return MySpan[index];
}
public static int Test6(int index)
{
if (index >= 0 && index < MySpan.Length)
return MySpan[index];
return 0;
}
public static int Test7(int index)
{
if (index < 0)
throw new Exception();
if (index >= MySpan.Length)
throw new ArgumentException();
return MySpan[index];
}
public static int Test8(int index)
{
if ((uint)index < 2) // 2 is less than MySpan.Length
return MySpan[index];
return 0;
}
public static int Test9(int index)
{
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]
throw new ArgumentException();
return MySpan[index];
}
} Unrelated pattern that got fixed too: int foo(uint i, int[] array)
{
if (i < array.Length)
return array[i];
return 0;
} if index is defined as unsigned - we don't need any casts to uint. |
e87d3b2
to
58f4f68
Compare
/azp run Fuzzlyn, Antigen |
Azure Pipelines successfully started running 2 pipeline(s). |
@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. |
…/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
…hecks-constant-len
@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.
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 |
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.