Skip to content

Commit

Permalink
Do not recost and rethread trees inside optRemoveRangeCheck (#69895)
Browse files Browse the repository at this point in the history
* 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.
  • Loading branch information
SingleAccretion authored May 27, 2022
1 parent 70fd5dc commit 1f303a1
Show file tree
Hide file tree
Showing 4 changed files with 26 additions and 17 deletions.
6 changes: 4 additions & 2 deletions src/coreclr/jit/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -5494,7 +5494,6 @@ class Compiler
GenTree* fgInsertCommaFormTemp(GenTree** ppTree, CORINFO_CLASS_HANDLE structType = nullptr);
GenTree* fgMakeMultiUse(GenTree** ppTree, CORINFO_CLASS_HANDLE structType = nullptr);

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

//-------- Determine the order in which the trees will be evaluated -------
GenTree* fgSetTreeSeq(GenTree* tree, bool isLIR = false);
public:
void fgSetStmtSeq(Statement* stmt);

private:
GenTree* fgSetTreeSeq(GenTree* tree, bool isLIR = false);
void fgSetBlockOrder(BasicBlock* block);

//------------------------- Morphing --------------------------------------
Expand Down
19 changes: 6 additions & 13 deletions src/coreclr/jit/optimizer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8107,10 +8107,12 @@ void Compiler::AddModifiedElemTypeAllContainingLoops(unsigned lnum, CORINFO_CLAS
// Return Value:
// Rewritten "check" - no-op if it has no side effects or the tree that contains them.
//
// Assumptions:
// This method is capable of removing checks of two kinds: COMMA-based and standalone top-level ones.
// In case of a COMMA-based check, "check" must be a non-null first operand of a non-null COMMA.
// In case of a standalone check, "comma" must be null and "check" - "stmt"'s root.
// Notes:
// This method is capable of removing checks of two kinds: COMMA-based and standalone top-level
// ones. In case of a COMMA-based check, "check" must be a non-null first operand of a non-null
// COMMA. In case of a standalone check, "comma" must be null and "check" - "stmt"'s root.
//
// Does not keep costs or node threading up to date, but does update side effect flags.
//
GenTree* Compiler::optRemoveRangeCheck(GenTreeBoundsChk* check, GenTree* comma, Statement* stmt)
{
Expand Down Expand Up @@ -8165,15 +8167,6 @@ GenTree* Compiler::optRemoveRangeCheck(GenTreeBoundsChk* check, GenTree* comma,

gtUpdateSideEffects(stmt, tree);

// Recalculate the GetCostSz(), etc...
gtSetStmtInfo(stmt);

// Re-thread the nodes if necessary
if (fgStmtListThreaded)
{
fgSetStmtSeq(stmt);
}

#ifdef DEBUG
if (verbose)
{
Expand Down
15 changes: 13 additions & 2 deletions src/coreclr/jit/rangecheck.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ RangeCheck::RangeCheck(Compiler* pCompiler)
, m_pCompiler(pCompiler)
, m_alloc(pCompiler->getAllocator(CMK_RangeCheck))
, m_nVisitBudget(MAX_VISIT_BUDGET)
, m_updateStmt(false)
{
}

Expand Down Expand Up @@ -255,6 +256,7 @@ void RangeCheck::OptimizeRangeCheck(BasicBlock* block, Statement* stmt, GenTree*
{
JITDUMP("Removing range check\n");
m_pCompiler->optRemoveRangeCheck(bndsChk, comma, stmt);
m_updateStmt = true;
return;
}
}
Expand Down Expand Up @@ -296,8 +298,8 @@ void RangeCheck::OptimizeRangeCheck(BasicBlock* block, Statement* stmt, GenTree*
{
JITDUMP("[RangeCheck::OptimizeRangeCheck] Between bounds\n");
m_pCompiler->optRemoveRangeCheck(bndsChk, comma, stmt);
m_updateStmt = true;
}
return;
}

void RangeCheck::Widen(BasicBlock* block, GenTree* tree, Range* pRange)
Expand Down Expand Up @@ -1545,14 +1547,23 @@ void RangeCheck::OptimizeRangeChecks()
{
for (Statement* const stmt : block->Statements())
{
m_updateStmt = false;

for (GenTree* const tree : stmt->TreeList())
{
if (IsOverBudget())
if (IsOverBudget() && !m_updateStmt)
{
return;
}

OptimizeRangeCheck(block, stmt, tree);
}

if (m_updateStmt)
{
m_pCompiler->gtSetStmtInfo(stmt);
m_pCompiler->fgSetStmtSeq(stmt);
}
}
}
}
3 changes: 3 additions & 0 deletions src/coreclr/jit/rangecheck.h
Original file line number Diff line number Diff line change
Expand Up @@ -650,4 +650,7 @@ class RangeCheck
// The number of nodes for which range is computed throughout the current method.
// When this limit is zero, we have exhausted all the budget to walk the ud-chain.
int m_nVisitBudget;

// Set to "true" whenever we remove a check and need to re-thread the statement.
bool m_updateStmt;
};

0 comments on commit 1f303a1

Please sign in to comment.