JIT: Fix range checks for "i >= 0 && i < cns" patterns #61569
Conversation
ghost
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT Issue Details@stephentoub noticed that JIT doesn't handle well range checks for arrays/spans of known lengths, e.g.: static ReadOnlySpan<byte> span => new byte[] { 1, 2, 3, 4 };
byte Test1(int i)
{
if ((uint)i < (uint)span.Length) // span.Length is folded into '4'
return span[i];
return 0;
}
const String MyStr = "test";
char Test2(int i)
{
if ((uint)i < (uint)MyStr.Length) // MyStr.Length is folded into '4'
return MyStr[i];
return ' ';
}For both It turns out then later these assertions are merged correctly in but then we just give up on it due to If my fix is correct, I then will change assertprop to emit exactly the same assertions for and cc @dotnet/jit-contrib @SingleAccretion
|
|
Example: static ReadOnlySpan<byte> span => new byte[] { 1, 2, 3, 4 };
static byte Test1(int i)
{
if (i > 0 && i < span.Length)
return span[i];
return 0;
}
const String MyStr = "hello";
char Test2(int i)
{
if (i > 0 && i < MyStr.Length)
return MyStr[i];
return ' ';
}Codegen diff (Main vs This PR): https://www.diffchecker.com/CeU2AlYV |
|
@dotnet/jit-contrib PTAL, a simple change. |
|
@BruceForstall @AndyAyersMS could some of you take a look please as it looks like you were the only jit team members who touched this area? Once this is merged I'll file a one more PR on top of this one to finally fix bound checks for scenarios like: static ReadOnlySpan<byte> span => new byte[] { 1, 2, 3, 4 };
byte Test1(int i)
{
if ((uint)i < (uint)span.Length)
return span[i]; |
Sorry, I don't have any experience with this code. However:
Why does this happen, and why is the fix not fixing that? It looks like we give up because it looks for the SSA def assignment of the index var, but probably doesn't find one (?) because it's an argument, so assumes it might overflow. Is that correct? It seems like if the lcl_var def is an argument, then it should return |
| assert(binop->OperIs(GT_ADD, GT_MUL, GT_LSH)); | ||
|
|
||
| GenTree* op1 = binop->gtGetOp1(); | ||
| GenTree* op2 = binop->gtGetOp2(); | ||
|
|
||
| JITDUMP("[RangeCheck::IsBinOpMonotonicallyIncreasing] [%06d], [%06d]\n", Compiler::dspTreeID(op1), |
There was a problem hiding this comment.
Hmm, it is possible to get there for defs that aren't just FIRST_SSA_NUM ones, but also non-LCL_VAR ones. I think it would be safer to check that what we are seeing here is really the def that comes from outside the method:
There was a problem hiding this comment.
@SingleAccretion not sure I understand, can you elaborate?
There was a problem hiding this comment.
(Sorry for the delay :( ).
The !m_pCompiler->lvaIsParameter(lcl->GetLclNum()); check permits us to say that for any SSA definition (1st, 2nd, 3rd, etc) of a parameter, we assert it will not overflow.
However, that seems like a dangerous assumption, because we only know that is the case for the first definition (one that comes from outside the method), so I was suggesting the check be strengthened to this:
if (ssaDef == nullptr)
{
// Parameter definitions that come from outside the method could not have overflown.
if ((lcl->GetSsaNum() == SsaConfig::FIRST_SSA_NUM) && m_pCompiler->lvaIsParameter(lcl->GetLclNum()))
{
return false;
}
return true;
}I believe it is the case today we will not end up here with ssaDef == nullptr and lcl->GetSsaNum() != SsaConfig::FIRST_SSA_NUM, but that's a fragile (and non-obvious) invariant to rely on.
There was a problem hiding this comment.
You could also assert in the return false clause that the ssa def comes from fgFirstBB.
@BruceForstall good question, it's now addressed, thanks! |
ghost
locked as resolved and limited conversation to collaborators
4 participants
@stephentoub noticed that JIT doesn't handle well range checks for arrays/spans of known lengths, e.g.:
For both
Test1andTest2we always emit range checks no matter how conditions are reversed and how manyuintcasts are added 🙁. Part of the problem that bothspan.LengthandMyStr.Lengthare folded into constants.It turns out
assertpropdoesn't emit any assertions for thoseJTRUE. However, it does emit them for patterns like this:then later these assertions are merged correctly in
rangecheck.cppintobut then we just give up on it due to
DoesOverflowreturning true for index node (which is a LCL_VAR argument).If my fix is correct, I then will change
optCreateJTrueBoundsAssertionto emit exactly the same assertions forand that will fix range checks for
if ((uint)i < span.Length)patterns too.cc @dotnet/jit-contrib @SingleAccretion
the only spmi diff is:
but I assume they will appear after the next step (in a separate PR)