Skip to content

Commit 1f303a1

Browse files
Do not recost and rethread trees inside optRemoveRangeCheck (#69895)
* Do not set order in "optRemoveRangeCheck" All but one caller of the method (RangeCheck) already do so in their "for (GenTree* node : stmt->TreeList())" loops, so it is not necessary. Additionally, re-threading the statement, when combined with "gtSetEvalOrder", can have the consequence of redirecting said loops, possibly causing them to miss some trees, which was observed in early propagation when working on removing "GT_INDEX". A few small diffs because we no longer recost when removing range checks in loop cloning, which is generally not necessary because cloning runs before the "global" costing is performed, except there is one quirk in "gtSetEvalOrder" which was looking as "compCurStmt", and that is not set during "fgSetBlockOrder". * Work around a TP regression Gets us back 0.13% (!!!) instructions according to PIN.
1 parent 70fd5dc commit 1f303a1

File tree

4 files changed

+26
-17
lines changed

4 files changed

+26
-17
lines changed

src/coreclr/jit/compiler.h

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5494,7 +5494,6 @@ class Compiler
54945494
GenTree* fgInsertCommaFormTemp(GenTree** ppTree, CORINFO_CLASS_HANDLE structType = nullptr);
54955495
GenTree* fgMakeMultiUse(GenTree** ppTree, CORINFO_CLASS_HANDLE structType = nullptr);
54965496

5497-
private:
54985497
// Recognize a bitwise rotation pattern and convert into a GT_ROL or a GT_ROR node.
54995498
GenTree* fgRecognizeAndMorphBitwiseRotation(GenTree* tree);
55005499
bool fgOperIsBitwiseRotationRoot(genTreeOps oper);
@@ -5506,8 +5505,11 @@ class Compiler
55065505
#endif
55075506

55085507
//-------- Determine the order in which the trees will be evaluated -------
5509-
GenTree* fgSetTreeSeq(GenTree* tree, bool isLIR = false);
5508+
public:
55105509
void fgSetStmtSeq(Statement* stmt);
5510+
5511+
private:
5512+
GenTree* fgSetTreeSeq(GenTree* tree, bool isLIR = false);
55115513
void fgSetBlockOrder(BasicBlock* block);
55125514

55135515
//------------------------- Morphing --------------------------------------

src/coreclr/jit/optimizer.cpp

Lines changed: 6 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -8107,10 +8107,12 @@ void Compiler::AddModifiedElemTypeAllContainingLoops(unsigned lnum, CORINFO_CLAS
81078107
// Return Value:
81088108
// Rewritten "check" - no-op if it has no side effects or the tree that contains them.
81098109
//
8110-
// Assumptions:
8111-
// This method is capable of removing checks of two kinds: COMMA-based and standalone top-level ones.
8112-
// In case of a COMMA-based check, "check" must be a non-null first operand of a non-null COMMA.
8113-
// In case of a standalone check, "comma" must be null and "check" - "stmt"'s root.
8110+
// Notes:
8111+
// This method is capable of removing checks of two kinds: COMMA-based and standalone top-level
8112+
// ones. In case of a COMMA-based check, "check" must be a non-null first operand of a non-null
8113+
// COMMA. In case of a standalone check, "comma" must be null and "check" - "stmt"'s root.
8114+
//
8115+
// Does not keep costs or node threading up to date, but does update side effect flags.
81148116
//
81158117
GenTree* Compiler::optRemoveRangeCheck(GenTreeBoundsChk* check, GenTree* comma, Statement* stmt)
81168118
{
@@ -8165,15 +8167,6 @@ GenTree* Compiler::optRemoveRangeCheck(GenTreeBoundsChk* check, GenTree* comma,
81658167

81668168
gtUpdateSideEffects(stmt, tree);
81678169

8168-
// Recalculate the GetCostSz(), etc...
8169-
gtSetStmtInfo(stmt);
8170-
8171-
// Re-thread the nodes if necessary
8172-
if (fgStmtListThreaded)
8173-
{
8174-
fgSetStmtSeq(stmt);
8175-
}
8176-
81778170
#ifdef DEBUG
81788171
if (verbose)
81798172
{

src/coreclr/jit/rangecheck.cpp

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ RangeCheck::RangeCheck(Compiler* pCompiler)
2424
, m_pCompiler(pCompiler)
2525
, m_alloc(pCompiler->getAllocator(CMK_RangeCheck))
2626
, m_nVisitBudget(MAX_VISIT_BUDGET)
27+
, m_updateStmt(false)
2728
{
2829
}
2930

@@ -255,6 +256,7 @@ void RangeCheck::OptimizeRangeCheck(BasicBlock* block, Statement* stmt, GenTree*
255256
{
256257
JITDUMP("Removing range check\n");
257258
m_pCompiler->optRemoveRangeCheck(bndsChk, comma, stmt);
259+
m_updateStmt = true;
258260
return;
259261
}
260262
}
@@ -296,8 +298,8 @@ void RangeCheck::OptimizeRangeCheck(BasicBlock* block, Statement* stmt, GenTree*
296298
{
297299
JITDUMP("[RangeCheck::OptimizeRangeCheck] Between bounds\n");
298300
m_pCompiler->optRemoveRangeCheck(bndsChk, comma, stmt);
301+
m_updateStmt = true;
299302
}
300-
return;
301303
}
302304

303305
void RangeCheck::Widen(BasicBlock* block, GenTree* tree, Range* pRange)
@@ -1545,14 +1547,23 @@ void RangeCheck::OptimizeRangeChecks()
15451547
{
15461548
for (Statement* const stmt : block->Statements())
15471549
{
1550+
m_updateStmt = false;
1551+
15481552
for (GenTree* const tree : stmt->TreeList())
15491553
{
1550-
if (IsOverBudget())
1554+
if (IsOverBudget() && !m_updateStmt)
15511555
{
15521556
return;
15531557
}
1558+
15541559
OptimizeRangeCheck(block, stmt, tree);
15551560
}
1561+
1562+
if (m_updateStmt)
1563+
{
1564+
m_pCompiler->gtSetStmtInfo(stmt);
1565+
m_pCompiler->fgSetStmtSeq(stmt);
1566+
}
15561567
}
15571568
}
15581569
}

src/coreclr/jit/rangecheck.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -650,4 +650,7 @@ class RangeCheck
650650
// The number of nodes for which range is computed throughout the current method.
651651
// When this limit is zero, we have exhausted all the budget to walk the ud-chain.
652652
int m_nVisitBudget;
653+
654+
// Set to "true" whenever we remove a check and need to re-thread the statement.
655+
bool m_updateStmt;
653656
};

0 commit comments

Comments
 (0)