Skip to content

Commit 25bd04c

Browse files
EgorBoCopilot
andauthored
Clean up in rangecheck (#129144)
Minor cleanup in `rangecheck.cpp`: - Range Check was trying to do what assertionprop already did - handle constant indices. Removed with zero diffs. - The previous code had a correctness issue - it called a slow SSA-based `GetRangeWorker` for Array's length but didn't use `DoesOverflow` that must be called for `GetRangeWorker`. Instead, I just call the VN-based `GetRangeFromAssertions` (that plays nicely with overflows) and it seems to be enough, a few minor regression, a minor TP improvement. > [!NOTE] > This PR was generated with the assistance of GitHub Copilot. [Diffs](https://dev.azure.com/dnceng-public/public/_build/results?buildId=1455184&view=ms.vss-build-web.run-extensions-tab) (from an incorrect call to `GetRangeWorker` without `DoesOverflow`) --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 8ac369d commit 25bd04c

3 files changed

Lines changed: 9 additions & 114 deletions

File tree

src/coreclr/jit/assertionprop.cpp

Lines changed: 0 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -1337,55 +1337,6 @@ AssertionIndex Compiler::optCreateAssertion(GenTree* op1, GenTree* op2, bool equ
13371337
return NO_ASSERTION_INDEX;
13381338
}
13391339

1340-
/*****************************************************************************
1341-
*
1342-
* If tree is a constant node holding an integral value, retrieve the value in
1343-
* pConstant. If the method returns true, pConstant holds the appropriate
1344-
* constant. Set "vnBased" to true to indicate local or global assertion prop.
1345-
* "pFlags" indicates if the constant is a handle marked by GTF_ICON_HDL_MASK.
1346-
*/
1347-
bool Compiler::optIsTreeKnownIntValue(bool vnBased, GenTree* tree, ssize_t* pConstant, GenTreeFlags* pFlags)
1348-
{
1349-
// Is Local assertion prop?
1350-
if (!vnBased)
1351-
{
1352-
if (tree->OperIs(GT_CNS_INT))
1353-
{
1354-
*pConstant = tree->AsIntCon()->IconValue();
1355-
*pFlags = tree->GetIconHandleFlag();
1356-
return true;
1357-
}
1358-
return false;
1359-
}
1360-
1361-
// Global assertion prop
1362-
ValueNum vn = vnStore->VNConservativeNormalValue(tree->gtVNPair);
1363-
if (!vnStore->IsVNConstant(vn))
1364-
{
1365-
return false;
1366-
}
1367-
1368-
// ValueNumber 'vn' indicates that this node evaluates to a constant
1369-
1370-
var_types vnType = vnStore->TypeOfVN(vn);
1371-
if (vnType == TYP_INT)
1372-
{
1373-
*pConstant = vnStore->ConstantValue<int>(vn);
1374-
*pFlags = vnStore->IsVNHandle(vn) ? vnStore->GetHandleFlags(vn) : GTF_EMPTY;
1375-
return true;
1376-
}
1377-
#ifdef TARGET_64BIT
1378-
else if (vnType == TYP_LONG)
1379-
{
1380-
*pConstant = vnStore->ConstantValue<INT64>(vn);
1381-
*pFlags = vnStore->IsVNHandle(vn) ? vnStore->GetHandleFlags(vn) : GTF_EMPTY;
1382-
return true;
1383-
}
1384-
#endif
1385-
1386-
return false;
1387-
}
1388-
13891340
/*****************************************************************************
13901341
*
13911342
* Given an assertion add it to the assertion table

src/coreclr/jit/compiler.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9075,7 +9075,6 @@ class Compiler
90759075
// Assertion prop data flow functions.
90769076
PhaseStatus optAssertionPropMain();
90779077
Statement* optVNAssertionPropCurStmt(BasicBlock* block, Statement* stmt);
9078-
bool optIsTreeKnownIntValue(bool vnBased, GenTree* tree, ssize_t* pConstant, GenTreeFlags* pIconFlags);
90799078
ASSERT_TP* optInitAssertionDataflowFlags();
90809079
ASSERT_TP* optComputeAssertionGen();
90819080

src/coreclr/jit/rangecheck.cpp

Lines changed: 9 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -272,73 +272,15 @@ void RangeCheck::OptimizeRangeCheck(BasicBlock* block, Statement* stmt, GenTree*
272272
return;
273273
}
274274

275-
GenTree* comma = treeParent->OperIs(GT_COMMA) ? treeParent : nullptr;
276-
GenTreeBoundsChk* bndsChk = tree->AsBoundsChk();
277-
m_preferredBound = m_compiler->vnStore->VNConservativeNormalValue(bndsChk->GetArrayLength()->gtVNPair);
278-
GenTree* treeIndex = bndsChk->GetIndex();
275+
GenTree* comma = treeParent->OperIs(GT_COMMA) ? treeParent : nullptr;
276+
GenTreeBoundsChk* bndsChk = tree->AsBoundsChk();
277+
GenTree* treeIndex = bndsChk->GetIndex();
279278

280279
// Take care of constant index first, like a[2], for example.
281-
ValueNum idxVn = m_compiler->vnStore->VNConservativeNormalValue(treeIndex->gtVNPair);
282-
ValueNum arrLenVn = m_compiler->vnStore->VNConservativeNormalValue(bndsChk->GetArrayLength()->gtVNPair);
283-
int arrSize = 0;
280+
ValueNum idxVn = m_compiler->optConservativeNormalVN(treeIndex);
281+
ValueNum arrLenVn = m_compiler->optConservativeNormalVN(bndsChk->GetArrayLength());
284282

285-
if (m_compiler->vnStore->IsVNConstant(arrLenVn))
286-
{
287-
ssize_t constVal = -1;
288-
GenTreeFlags iconFlags = GTF_EMPTY;
289-
290-
if (m_compiler->optIsTreeKnownIntValue(true, bndsChk->GetArrayLength(), &constVal, &iconFlags))
291-
{
292-
arrSize = (int)constVal;
293-
}
294-
}
295-
else
296-
{
297-
arrSize = GetArrLength(arrLenVn);
298-
299-
// if we can't find the array length, see if there
300-
// are any assertions about the array size we can use to get a minimum length
301-
if (arrSize <= 0)
302-
{
303-
JITDUMP("Looking for array size assertions for: " FMT_VN "\n", arrLenVn);
304-
Range arrLength = Range(Limit(Limit::keDependent));
305-
MergeEdgeAssertions(m_compiler, arrLenVn, arrLenVn, block->bbAssertionIn, &arrLength);
306-
if (arrLength.lLimit.IsConstant())
307-
{
308-
arrSize = arrLength.lLimit.GetConstant();
309-
}
310-
else
311-
{
312-
// Fast path didn't find anything - do the slow SSA-based search.
313-
arrLength = GetRangeWorker(block, bndsChk->GetArrayLength(), false DEBUGARG(0));
314-
if (arrLength.lLimit.IsConstant())
315-
{
316-
arrSize = arrLength.lLimit.GetConstant();
317-
}
318-
}
319-
}
320-
}
321-
322-
JITDUMP("ArrSize for lengthVN:%03X = %d\n", arrLenVn, arrSize);
323-
if (m_compiler->vnStore->IsVNConstant(idxVn) && (arrSize > 0))
324-
{
325-
ssize_t idxVal = -1;
326-
GenTreeFlags iconFlags = GTF_EMPTY;
327-
if (!m_compiler->optIsTreeKnownIntValue(true, treeIndex, &idxVal, &iconFlags))
328-
{
329-
return;
330-
}
331-
332-
JITDUMP("[RangeCheck::OptimizeRangeCheck] Is index %d in <0, arrLenVn " FMT_VN " sz:%d>.\n", idxVal, arrLenVn,
333-
arrSize);
334-
if ((idxVal < arrSize) && (idxVal >= 0))
335-
{
336-
JITDUMP("Removing range check\n");
337-
m_compiler->optRemoveRangeCheck(bndsChk, comma, stmt);
338-
m_updateStmt = true;
339-
return;
340-
}
341-
}
283+
m_preferredBound = arrLenVn;
342284

343285
// Special case: arr[arr.Length - CNS] if we know that arr.Length >= CNS
344286
// We assume that SUB(x, CNS) is canonized into ADD(x, -CNS)
@@ -428,6 +370,9 @@ void RangeCheck::OptimizeRangeCheck(BasicBlock* block, Statement* stmt, GenTree*
428370
return;
429371
}
430372

373+
Range arrSizeRng = GetRangeFromAssertions(m_compiler, arrLenVn, block->bbAssertionIn);
374+
int arrSize = arrSizeRng.LowerLimit().GetConstant();
375+
431376
// Is the range between the lower and upper bound values.
432377
if (BetweenBounds(range, bndsChk->GetArrayLength(), arrSize))
433378
{

0 commit comments

Comments
 (0)