Skip to content

Commit a075769

Browse files
EgorBoCopilot
andauthored
Misc improvements for rangecheck (#129101)
- Fix unsound bounds in `RangeOps::Or` (could exclude reachable values); use `max`-based lower/upper bounds - existing bug: `[3..5] | [4..7] -> lo = 3|4 = 7, hi = 5|7 = 7 (claims [7..7])` - Add `UDIV` constant-range support to `GetRangeFromAssertionsWorker` - Populate `m_isVNNeverNegative` on checked-bound / VN-relop assertions - Restrict the never-negative `O2K_VN_ADD_CNS` range deduction to LT/LE relops (GT/GE were unsound) [Diffs](https://dev.azure.com/dnceng-public/public/_build/results?buildId=1453272&view=ms.vss-build-web.run-extensions-tab) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 4daceb8 commit a075769

3 files changed

Lines changed: 74 additions & 18 deletions

File tree

src/coreclr/jit/compiler.h

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8976,13 +8976,14 @@ class Compiler
89768976
assert(op1VN != ValueNumStore::NoVN);
89778977
assert(checkedBndVN != ValueNumStore::NoVN);
89788978

8979-
AssertionDsc dsc = CreateEmptyAssertion(comp);
8980-
dsc.m_assertionKind = FromVNFunc(relop);
8981-
dsc.m_op1.m_kind = O1K_VN;
8982-
dsc.m_op1.m_vn = op1VN;
8983-
dsc.m_op2.m_kind = O2K_VN_ADD_CNS;
8984-
dsc.m_op2.m_vn = checkedBndVN;
8985-
dsc.m_op2.m_icon.m_iconVal = cns;
8979+
AssertionDsc dsc = CreateEmptyAssertion(comp);
8980+
dsc.m_assertionKind = FromVNFunc(relop);
8981+
dsc.m_op1.m_kind = O1K_VN;
8982+
dsc.m_op1.m_vn = op1VN;
8983+
dsc.m_op2.m_kind = O2K_VN_ADD_CNS;
8984+
dsc.m_op2.m_vn = checkedBndVN;
8985+
dsc.m_op2.m_icon.m_iconVal = cns;
8986+
dsc.m_op2.m_isVNNeverNegative = comp->vnStore->IsVNNeverNegative(checkedBndVN);
89868987
return dsc;
89878988
}
89888989

@@ -9021,7 +9022,7 @@ class Compiler
90219022
dsc.m_op2.m_kind = O2K_VN_ADD_CNS;
90229023
dsc.m_op2.m_vn = op2VN;
90239024
dsc.m_op2.m_icon.m_iconVal = 0; // TODO-CQ: consider decomposing VN to VN* + CNS if beneficial.
9024-
dsc.m_op2.m_isVNNeverNegative = false;
9025+
dsc.m_op2.m_isVNNeverNegative = comp->vnStore->IsVNNeverNegative(op2VN);
90259026
return dsc;
90269027
}
90279028
};

src/coreclr/jit/rangecheck.cpp

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -791,6 +791,7 @@ Range RangeCheck::GetRangeFromAssertionsWorker(
791791
case VNF_RSH:
792792
case VNF_RSZ:
793793
case VNF_UMOD:
794+
case VNF_UDIV:
794795
{
795796
// Get ranges of both operands and perform the same operation on the ranges.
796797
Range r1 = GetRangeFromAssertionsWorker(comp, funcApp.GetArg(0), assertions, --budget, visited);
@@ -825,6 +826,9 @@ Range RangeCheck::GetRangeFromAssertionsWorker(
825826
case VNF_UMOD:
826827
binOpResult = RangeOps::UnsignedMod(r1, r2);
827828
break;
829+
case VNF_UDIV:
830+
binOpResult = RangeOps::UnsignedDivide(r1, r2);
831+
break;
828832
default:
829833
unreached();
830834
}
@@ -1230,7 +1234,13 @@ void RangeCheck::MergeEdgeAssertionsWorker(Compiler* comp
12301234
// Example: "(uint)normalLclVN < span.Length" means normalLclVN's range is [0..INT32_MAX-1]
12311235
// Example: "(uint)normalLclVN <= array.Length" means normalLclVN's range is [0..Array.MaxLength]
12321236
//
1233-
else if (!canUseCheckedBounds && curAssertion.IsRelop() && (curAssertion.GetOp1().GetVN() == normalLclVN) &&
1237+
// This is restricted to upper-bound (LT/LE) relops: we substitute op2 with its upper bound
1238+
// (maxValue), which is only valid when op2 bounds normalLclVN from above. GT/GE relops (where
1239+
// op2 is a lower bound) would need op2's lower bound instead, so they fall through to the
1240+
// general "X <relop> Y" branch below.
1241+
else if (!canUseCheckedBounds &&
1242+
curAssertion.KindIs(Compiler::OAK_LT, Compiler::OAK_LE, Compiler::OAK_LT_UN, Compiler::OAK_LE_UN) &&
1243+
(curAssertion.GetOp1().GetVN() == normalLclVN) &&
12341244
(curAssertion.GetOp2().KindIs(Compiler::O2K_VN_ADD_CNS)) &&
12351245
(curAssertion.GetOp2().IsVNNeverNegative()) && (curAssertion.GetOp2().GetCns() == 0))
12361246
{

src/coreclr/jit/rangecheck.h

Lines changed: 54 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -443,19 +443,39 @@ struct RangeOps
443443

444444
static Range Or(const Range& r1, const Range& r2)
445445
{
446-
// For OR we require both operands to be constant to produce a constant result.
447-
// No useful information can be derived if only one operand is constant.
446+
// For OR we require both operands to be non-negative constant ranges.
448447
//
449448
// Example: [0..3] | [1..255] = [1..255]
450449
// [X..Y] | [1..255] = [unknown..unknown]
451450
//
452-
return ApplyRangeOp(r1, r2, [](const Limit& a, const Limit& b) {
453-
if (a.IsConstant() && b.IsConstant() && (a.GetConstant() >= 0) && (b.GetConstant() >= 0))
454-
{
455-
return Limit(Limit::keConstant, a.GetConstant() | b.GetConstant());
456-
}
457-
return Limit(Limit::keUnknown);
458-
});
451+
if (!r1.IsConstantRange() || !r2.IsConstantRange())
452+
{
453+
return Range(Limit(Limit::keUnknown));
454+
}
455+
456+
const int r1lo = r1.LowerLimit().GetConstant();
457+
const int r1hi = r1.UpperLimit().GetConstant();
458+
const int r2lo = r2.LowerLimit().GetConstant();
459+
const int r2hi = r2.UpperLimit().GetConstant();
460+
461+
if ((r1lo < 0) || (r2lo < 0))
462+
{
463+
return Range(Limit(Limit::keUnknown));
464+
}
465+
466+
// a | b >= max(a, b), so max(r1lo, r2lo) is a sound lower bound.
467+
const int lo = max(r1lo, r2lo);
468+
469+
// a | b cannot set any bit above the most-significant set bit of max(r1hi, r2hi), so it is
470+
// bounded above by that value with all lower bits filled in (smallest 2^k - 1 >= the max).
471+
int hi = max(r1hi, r2hi);
472+
hi |= hi >> 1;
473+
hi |= hi >> 2;
474+
hi |= hi >> 4;
475+
hi |= hi >> 8;
476+
hi |= hi >> 16;
477+
478+
return Range(Limit(Limit::keConstant, lo), Limit(Limit::keConstant, hi));
459479
}
460480

461481
static Range And(const Range& r1, const Range& r2)
@@ -501,6 +521,31 @@ struct RangeOps
501521
return Range(Limit(Limit::keUnknown));
502522
}
503523

524+
static Range UnsignedDivide(const Range& r1, const Range& r2)
525+
{
526+
// We only handle constant ranges for both operands.
527+
if (!r1.IsConstantRange() || !r2.IsConstantRange())
528+
{
529+
return Range(Limit(Limit::keUnknown));
530+
}
531+
532+
const int numLo = r1.LowerLimit().GetConstant();
533+
const int numHi = r1.UpperLimit().GetConstant();
534+
const int divLo = r2.LowerLimit().GetConstant();
535+
const int divHi = r2.UpperLimit().GetConstant();
536+
537+
// Require a non-negative dividend and a strictly-positive divisor so that the signed
538+
// [lo..hi] bounds coincide with their unsigned interpretation (and no division by zero).
539+
if ((numLo < 0) || (divLo <= 0))
540+
{
541+
return Range(Limit(Limit::keUnknown));
542+
}
543+
544+
// (uint)x / (uint)y is maximized by the largest dividend over the smallest divisor and
545+
// minimized by the smallest dividend over the largest divisor.
546+
return Range(Limit(Limit::keConstant, numLo / divHi), Limit(Limit::keConstant, numHi / divLo));
547+
}
548+
504549
// Given two ranges "r1" and "r2", do a Phi merge. If "monIncreasing" is true,
505550
// then ignore the dependent variables for the lower bound but not for the upper bound.
506551
static Range Merge(const Range& r1, const Range& r2, bool monIncreasing)

0 commit comments

Comments
 (0)