Skip to content

Commit f46c3dd

Browse files
AndyAyersMSAndyAyersMS
committed
JIT: clone "i != arr.Length" loops to eliminate bounds checks
Loops with `i != end` previously kept their per-iteration bounds checks while the equivalent `<` / `>` forms did not, because loop cloning bailed on GT_NE. Recognize GT_NE with stride exactly +/-1 as an increasing or decreasing loop in NaturalLoopIterInfo, extend optDeriveLoopCloningConditions to emit the right per-access and zero-trip conditions for GT_NE, and add a new "init RELOP end" cloning condition so that a misordered init (which for GT_NE would wrap the IV through the type rather than exit at the first test) falls back to the slow path. Builds on #129176 which fixes the latent decreasing-loop soundness gap. A follow-on change to assertion prop / range check will catch the remaining NE-loop BCE cases that fall outside loop cloning (e.g. loops the cloning heuristic rejects). Contributes to #84697.
1 parent 25bd04c commit f46c3dd

4 files changed

Lines changed: 234 additions & 13 deletions

File tree

‎src/coreclr/jit/flowgraph.cpp‎

Lines changed: 26 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6848,9 +6848,20 @@ genTreeOps NaturalLoopIterInfo::TestOper()
68486848
bool NaturalLoopIterInfo::IsIncreasingLoop()
68496849
{
68506850
// Increasing loop is the one that has "+=" increment operation and "< or <=" limit check.
6851-
bool isLessThanLimitCheck = GenTree::StaticOperIs(TestOper(), GT_LT, GT_LE);
6852-
return (isLessThanLimitCheck &&
6853-
(((IterOper() == GT_ADD) && (IterConst() > 0)) || ((IterOper() == GT_SUB) && (IterConst() < 0))));
6851+
// We also recognize "!=" against a limit when the IV step is exactly +1 (or -1 with GT_SUB):
6852+
// such loops visit indices [init, limit) provided that "init <= limit" holds on entry.
6853+
// Stride must be ±1 to avoid parity/overflow issues where the IV could skip past the limit
6854+
// (e.g. "for (i = 0; i != 5; i += 2)" never terminates and wraps around).
6855+
const genTreeOps testOp = TestOper();
6856+
if (GenTree::StaticOperIs(testOp, GT_LT, GT_LE))
6857+
{
6858+
return (((IterOper() == GT_ADD) && (IterConst() > 0)) || ((IterOper() == GT_SUB) && (IterConst() < 0)));
6859+
}
6860+
if (testOp == GT_NE)
6861+
{
6862+
return ((IterOper() == GT_ADD) && (IterConst() == 1)) || ((IterOper() == GT_SUB) && (IterConst() == -1));
6863+
}
6864+
return false;
68546865
}
68556866

68566867
//------------------------------------------------------------------------
@@ -6864,9 +6875,18 @@ bool NaturalLoopIterInfo::IsDecreasingLoop()
68646875
{
68656876
// Decreasing loop is the one that has "-=" decrement operation and "> or >=" limit check. If the operation is
68666877
// "+=", make sure the constant is negative to give an effect of decrementing the iterator.
6867-
bool isGreaterThanLimitCheck = GenTree::StaticOperIs(TestOper(), GT_GT, GT_GE);
6868-
return (isGreaterThanLimitCheck &&
6869-
(((IterOper() == GT_ADD) && (IterConst() < 0)) || ((IterOper() == GT_SUB) && (IterConst() > 0))));
6878+
// As with IsIncreasingLoop, we also recognize "!=" against a limit when the IV step is exactly -1
6879+
// (or +1 with GT_SUB); the stride must be exactly +/-1 to avoid parity/overflow issues.
6880+
const genTreeOps testOp = TestOper();
6881+
if (GenTree::StaticOperIs(testOp, GT_GT, GT_GE))
6882+
{
6883+
return (((IterOper() == GT_ADD) && (IterConst() < 0)) || ((IterOper() == GT_SUB) && (IterConst() > 0)));
6884+
}
6885+
if (testOp == GT_NE)
6886+
{
6887+
return ((IterOper() == GT_ADD) && (IterConst() == -1)) || ((IterOper() == GT_SUB) && (IterConst() == 1));
6888+
}
6889+
return false;
68706890
}
68716891

68726892
//------------------------------------------------------------------------

‎src/coreclr/jit/loopcloning.cpp‎

Lines changed: 74 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1189,10 +1189,11 @@ bool Compiler::optDeriveLoopCloningConditions(FlowGraphNaturalLoop* loop, LoopCl
11891189
}
11901190

11911191
NaturalLoopIterInfo* iterInfo = context->GetLoopIterInfo(loop->GetIndex());
1192-
// Note we see cases where the test oper is NE (array.Len) which we could handle
1193-
// with some extra care.
1192+
// Loop tests we can reason about for cloning: ordered relops (LT/LE/GT/GE) and
1193+
// NE limits (treated as LT/GT-equivalent when stride is exactly +/-1; see
1194+
// NaturalLoopIterInfo::IsIncreasingLoop/IsDecreasingLoop).
11941195
//
1195-
if (!GenTree::StaticOperIs(iterInfo->TestOper(), GT_LT, GT_LE, GT_GT, GT_GE))
1196+
if (!GenTree::StaticOperIs(iterInfo->TestOper(), GT_LT, GT_LE, GT_GT, GT_GE, GT_NE))
11961197
{
11971198
// We can't reason about how this loop iterates
11981199
return false;
@@ -1312,9 +1313,17 @@ bool Compiler::optDeriveLoopCloningConditions(FlowGraphNaturalLoop* loop, LoopCl
13121313
}
13131314

13141315
// TestOper() returns the stays-in-loop relop in IV-on-lhs form, already
1315-
// adjusted for IsReversed and ExitedOnTrue.
1316-
LC_Condition zeroTrip(iterInfo->TestOper(), LC_Expr(initIdent), LC_Expr(limitIdent),
1317-
iterInfo->TestTree->IsUnsigned());
1316+
// adjusted for IsReversed and ExitedOnTrue. For GT_NE we substitute an
1317+
// ordered relop (LT for increasing, GT for decreasing) so that the
1318+
// runtime guard strictly orders init and limit. Using the raw "init !=
1319+
// limit" form would let a misordered init pass while the fast clone
1320+
// (with bounds checks removed) wraps the IV through the type.
1321+
genTreeOps zeroTripOp = iterInfo->TestOper();
1322+
if (zeroTripOp == GT_NE)
1323+
{
1324+
zeroTripOp = iterInfo->IsIncreasingLoop() ? GT_LT : GT_GT;
1325+
}
1326+
LC_Condition zeroTrip(zeroTripOp, LC_Expr(initIdent), LC_Expr(limitIdent), iterInfo->TestTree->IsUnsigned());
13181327
context->EnsureConditions(loop->GetIndex())->Push(zeroTrip);
13191328
JITDUMP("Added zero-trip guard cloning condition\n");
13201329
}
@@ -1427,20 +1436,30 @@ bool Compiler::optDeriveLoopCloningConditions(FlowGraphNaturalLoop* loop, LoopCl
14271436
// GT_LT loop test: (start < end) ==> (end <= arrLen)
14281437
// GT_LE loop test: (start <= end) ==> (end < arrLen)
14291438
//
1439+
// GT_NE loop test (stride = +/-1; see IsIncreasing/DecreasingLoop):
1440+
// For increasing: visited indices are [init..end-1] => guard end <= arrLen (same as LT).
1441+
// `ident` is the loop's end value, so the per-access condition is (end <= arrLen).
1442+
// For decreasing: visited indices are [end+1..init] => guard init < arrLen (same as GT).
1443+
// `ident` is the loop's init value (set in the init-conditions section above and not
1444+
// overwritten by the GT_NE limit branch), so the per-access condition is
1445+
// (init < arrLen). This is why the switch below maps decreasing GT_NE to GT_LT.
1446+
//
14301447
// Decreasing loops
14311448
// Always check if iter var is less than array length.
14321449
genTreeOps opLimitCondition;
14331450
switch (iterInfo->TestOper())
14341451
{
14351452
case GT_LT:
1436-
14371453
opLimitCondition = GT_LE;
14381454
break;
14391455
case GT_LE:
14401456
case GT_GE:
14411457
case GT_GT:
14421458
opLimitCondition = GT_LT;
14431459
break;
1460+
case GT_NE:
1461+
opLimitCondition = isIncreasingLoop ? GT_LE : GT_LT;
1462+
break;
14441463
default:
14451464
unreached();
14461465
}
@@ -1496,6 +1515,54 @@ bool Compiler::optDeriveLoopCloningConditions(FlowGraphNaturalLoop* loop, LoopCl
14961515
}
14971516
}
14981517

1518+
// For GT_NE loops (with stride exactly +/-1; see IsIncreasing/DecreasingLoop),
1519+
// the cloned fast path preserves the "i != limit" exit test. If the IV starts
1520+
// past the limit, the loop would wrap around the type and access arbitrary
1521+
// memory because the fast path has its bounds checks removed. Guard the fast
1522+
// path with an ordered "init RELOP limit" condition (init <= limit for
1523+
// increasing, init >= limit for decreasing) so that a misordered init falls
1524+
// back to the slow path with bounds checks.
1525+
if (iterInfo->TestOper() == GT_NE)
1526+
{
1527+
LC_Ident neInitIdent;
1528+
if (iterInfo->HasConstInit)
1529+
{
1530+
assert(iterInfo->ConstInitValue >= 0);
1531+
neInitIdent = LC_Ident::CreateConst(static_cast<unsigned>(iterInfo->ConstInitValue));
1532+
}
1533+
else
1534+
{
1535+
const unsigned initLcl = iterInfo->IterVar;
1536+
assert(genActualTypeIsInt(lvaGetDesc(initLcl)));
1537+
neInitIdent = LC_Ident::CreateVar(initLcl, iterInfo->Iterator()->TypeGet());
1538+
}
1539+
1540+
LC_Ident neLimitIdent;
1541+
if (iterInfo->HasConstLimit)
1542+
{
1543+
const int limit = iterInfo->ConstLimit();
1544+
assert(limit >= 0);
1545+
neLimitIdent = LC_Ident::CreateConst(static_cast<unsigned>(limit));
1546+
}
1547+
else if (iterInfo->HasInvariantLocalLimit)
1548+
{
1549+
const unsigned limitLcl = iterInfo->VarLimit();
1550+
assert(genActualTypeIsInt(lvaGetDesc(limitLcl)));
1551+
neLimitIdent = LC_Ident::CreateVar(limitLcl, iterInfo->Limit()->TypeGet());
1552+
}
1553+
else
1554+
{
1555+
assert(iterInfo->HasArrayLengthLimit);
1556+
assert(limitArrIndex != nullptr);
1557+
neLimitIdent = LC_Ident::CreateArrAccess(LC_Array(LC_Array::Jagged, limitArrIndex, LC_Array::ArrLen));
1558+
}
1559+
1560+
const genTreeOps cmpOp = isIncreasingLoop ? GT_LE : GT_GE;
1561+
LC_Condition initLimitCond(cmpOp, LC_Expr(neInitIdent), LC_Expr(neLimitIdent));
1562+
context->EnsureConditions(loop->GetIndex())->Push(initLimitCond);
1563+
JITDUMP("Added NE init-vs-limit cloning condition\n");
1564+
}
1565+
14991566
JITDUMP("Conditions: ");
15001567
DBEXEC(verbose, context->PrintConditions(loop->GetIndex()));
15011568
JITDUMP("\n");

‎src/tests/JIT/Regression/JitBlue/Runtime_84697/Runtime_84697.cs‎

Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,133 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
4+
namespace Runtime_84697;
5+
6+
using System;
7+
using System.Runtime.CompilerServices;
8+
using Xunit;
9+
10+
public class Runtime_84697
11+
{
12+
[Fact]
13+
public static int TestEntryPoint()
14+
{
15+
int ok = 0;
16+
17+
byte[] bytes = new byte[10];
18+
for (int i = 0; i < bytes.Length; i++) bytes[i] = (byte)i;
19+
20+
if (SumByteArrayNE(bytes) == 45) ok++;
21+
if (SumByteArrayLT(bytes) == 45) ok++;
22+
if (SumByteArrayNE(Array.Empty<byte>()) == 0) ok++;
23+
if (SumStringNE("hello") == 'h' + 'e' + 'l' + 'l' + 'o') ok++;
24+
if (SumSpanNE(new int[] { 1, 2, 3, 4, 5 }) == 15) ok++;
25+
if (SumNEDecreasing(bytes) == 45) ok++;
26+
if (SumNEStride2(bytes) == 0 + 2 + 4 + 6 + 8) ok++;
27+
if (SumNEFromOffset(bytes, 3) == 3 + 4 + 5 + 6 + 7 + 8 + 9) ok++;
28+
if (SumNEFromOffset(bytes, bytes.Length) == 0) ok++;
29+
30+
// Soundness: when init > length and stride is +1, the NE loop would wrap
31+
// around without bounds checks if loop cloning got the entry guard wrong.
32+
// The slow path must run and throw IndexOutOfRangeException.
33+
bool threw = false;
34+
try
35+
{
36+
SumNEFromOffset(bytes, 100);
37+
}
38+
catch (IndexOutOfRangeException)
39+
{
40+
threw = true;
41+
}
42+
if (threw) ok++;
43+
44+
// Soundness: a decreasing "i != someLen" loop indexing a different array.
45+
// The loop visits init, init-1, ... and could OOB-read accessArr if a
46+
// fast clone bypasses the access array's bounds checks. Whether or not
47+
// the loop is cloned, the expected behavior is throwing IndexOutOfRange.
48+
bool threw2 = false;
49+
try
50+
{
51+
SumDecreasingNETwoArrays(new byte[1], new byte[5], 1000);
52+
}
53+
catch (IndexOutOfRangeException)
54+
{
55+
threw2 = true;
56+
}
57+
if (threw2) ok++;
58+
59+
return ok == 11 ? 100 : 1;
60+
}
61+
62+
[MethodImpl(MethodImplOptions.NoInlining)]
63+
private static int SumByteArrayNE(byte[] src)
64+
{
65+
int sum = 0;
66+
for (int i = 0; i != src.Length; i++)
67+
sum += src[i];
68+
return sum;
69+
}
70+
71+
[MethodImpl(MethodImplOptions.NoInlining)]
72+
private static int SumByteArrayLT(byte[] src)
73+
{
74+
int sum = 0;
75+
for (int i = 0; i < src.Length; i++)
76+
sum += src[i];
77+
return sum;
78+
}
79+
80+
[MethodImpl(MethodImplOptions.NoInlining)]
81+
private static int SumStringNE(string s)
82+
{
83+
int sum = 0;
84+
for (int i = 0; i != s.Length; i++)
85+
sum += s[i];
86+
return sum;
87+
}
88+
89+
[MethodImpl(MethodImplOptions.NoInlining)]
90+
private static int SumSpanNE(ReadOnlySpan<int> sp)
91+
{
92+
int sum = 0;
93+
for (int i = 0; i != sp.Length; i++)
94+
sum += sp[i];
95+
return sum;
96+
}
97+
98+
[MethodImpl(MethodImplOptions.NoInlining)]
99+
private static int SumNEDecreasing(byte[] src)
100+
{
101+
int sum = 0;
102+
for (int i = src.Length - 1; i != -1; i--)
103+
sum += src[i];
104+
return sum;
105+
}
106+
107+
[MethodImpl(MethodImplOptions.NoInlining)]
108+
private static int SumNEStride2(byte[] src)
109+
{
110+
int sum = 0;
111+
for (int i = 0; i != src.Length; i += 2)
112+
sum += src[i];
113+
return sum;
114+
}
115+
116+
[MethodImpl(MethodImplOptions.NoInlining)]
117+
private static int SumNEFromOffset(byte[] src, int start)
118+
{
119+
int sum = 0;
120+
for (int i = start; i != src.Length; i++)
121+
sum += src[i];
122+
return sum;
123+
}
124+
125+
[MethodImpl(MethodImplOptions.NoInlining)]
126+
private static int SumDecreasingNETwoArrays(byte[] limitArr, byte[] accessArr, int startIdx)
127+
{
128+
int sum = 0;
129+
for (int i = startIdx; i != limitArr.Length; i--)
130+
sum += accessArr[i];
131+
return sum;
132+
}
133+
}

‎src/tests/JIT/Regression/Regression_o_3.csproj‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,7 @@
9090
<Compile Include="JitBlue\Runtime_83387\Runtime_83387.cs" />
9191
<Compile Include="JitBlue\Runtime_83941\Runtime_83941.cs" />
9292
<Compile Include="JitBlue\Runtime_84693\Runtime_84693.cs" />
93+
<Compile Include="JitBlue\Runtime_84697\Runtime_84697.cs" />
9394
<Compile Include="JitBlue\Runtime_85225\Runtime_85225.cs" />
9495
<Compile Include="JitBlue\Runtime_85226\Runtime_85226.cs" />
9596
<Compile Include="JitBlue\Runtime_85382\Runtime_85382.cs" />

0 commit comments

Comments
 (0)