Skip to content

Commit

Permalink
JIT: Disallow SCEV analysis of non-values (#108603)
Browse files Browse the repository at this point in the history
SCEV analysis would allow stores either via simple add-rec handling or
by forwarding to the data of the store. Given that stores are not values
this is odd behavior that has bitten me while consuming the analysis
package.

Switch the analysis to return nullptr for stores; instead add handling
for `GT_PHI` in the places where it was needed.
  • Loading branch information
jakobbotsch authored Oct 8, 2024
1 parent 589f733 commit 770df10
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 30 deletions.
4 changes: 2 additions & 2 deletions src/coreclr/jit/inductionvariableopts.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -779,7 +779,7 @@ bool Compiler::optWidenIVs(ScalarEvolutionContext& scevContext,
JITDUMP("\n");
DISPSTMT(stmt);

Scev* scev = scevContext.Analyze(loop->GetHeader(), stmt->GetRootNode());
Scev* scev = scevContext.Analyze(loop->GetHeader(), stmt->GetRootNode()->AsLclVarCommon()->Data());
if (scev == nullptr)
{
JITDUMP(" Could not analyze header PHI\n");
Expand Down Expand Up @@ -1417,7 +1417,7 @@ bool StrengthReductionContext::TryStrengthReduce()
DISPSTMT(stmt);

GenTreeLclVarCommon* primaryIVLcl = stmt->GetRootNode()->AsLclVarCommon();
Scev* candidate = m_scevContext.Analyze(m_loop->GetHeader(), primaryIVLcl);
Scev* candidate = m_scevContext.Analyze(m_loop->GetHeader(), primaryIVLcl->Data());
if (candidate == nullptr)
{
JITDUMP(" Could not analyze header PHI\n");
Expand Down
62 changes: 38 additions & 24 deletions src/coreclr/jit/scev.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -563,25 +563,18 @@ Scev* ScalarEvolutionContext::AnalyzeNew(BasicBlock* block, GenTree* tree, int d
return nullptr;
}

return Analyze(ssaDsc->GetBlock(), ssaDsc->GetDefNode(), depth + 1);
return Analyze(ssaDsc->GetBlock(), ssaDsc->GetDefNode()->Data(), depth + 1);
}
case GT_STORE_LCL_VAR:
case GT_PHI:
{
GenTreeLclVarCommon* store = tree->AsLclVarCommon();
GenTree* data = store->Data();
if (!data->OperIs(GT_PHI))
{
return Analyze(block, data, depth + 1);
}

if (block != m_loop->GetHeader())
{
return nullptr;
}

// We have a phi def for the current loop. Look for a primary
// induction variable.
GenTreePhi* phi = data->AsPhi();
GenTreePhi* phi = tree->AsPhi();
GenTreePhiArg* enterSsa = nullptr;
GenTreePhiArg* backedgeSsa = nullptr;

Expand All @@ -606,7 +599,7 @@ Scev* ScalarEvolutionContext::AnalyzeNew(BasicBlock* block, GenTree* tree, int d

ScevLocal* enterScev = NewLocal(enterSsa->GetLclNum(), enterSsa->GetSsaNum());

LclVarDsc* dsc = m_comp->lvaGetDesc(store);
LclVarDsc* dsc = m_comp->lvaGetDesc(enterSsa->GetLclNum());
LclSsaVarDsc* ssaDsc = dsc->GetPerSsaData(backedgeSsa->GetSsaNum());

if (ssaDsc->GetDefNode() == nullptr)
Expand All @@ -615,7 +608,7 @@ Scev* ScalarEvolutionContext::AnalyzeNew(BasicBlock* block, GenTree* tree, int d
return nullptr;
}

if (ssaDsc->GetDefNode()->GetLclNum() != store->GetLclNum())
if (ssaDsc->GetDefNode()->GetLclNum() != enterSsa->GetLclNum())
{
assert(dsc->lvIsStructField && ssaDsc->GetDefNode()->GetLclNum() == dsc->lvParentLcl);
return nullptr;
Expand All @@ -625,7 +618,7 @@ Scev* ScalarEvolutionContext::AnalyzeNew(BasicBlock* block, GenTree* tree, int d

// Try simple but most common case first, where we have a direct
// add recurrence like i = i + 1.
Scev* simpleAddRec = CreateSimpleAddRec(store, enterScev, ssaDsc->GetBlock(), ssaDsc->GetDefNode()->Data());
Scev* simpleAddRec = CreateSimpleAddRec(phi, enterScev, ssaDsc->GetBlock(), ssaDsc->GetDefNode()->Data());
if (simpleAddRec != nullptr)
{
return simpleAddRec;
Expand Down Expand Up @@ -670,8 +663,8 @@ Scev* ScalarEvolutionContext::AnalyzeNew(BasicBlock* block, GenTree* tree, int d
// single operand is the recurrence, we can represent it as an add
// recurrence. See MakeAddRecFromRecursiveScev for the details.
//
ScevConstant* symbolicAddRec = NewConstant(data->TypeGet(), 0xdeadbeef);
m_ephemeralCache.Emplace(store, symbolicAddRec);
ScevConstant* symbolicAddRec = NewConstant(phi->TypeGet(), 0xdeadbeef);
m_ephemeralCache.Emplace(phi, symbolicAddRec);

Scev* result;
if (m_usingEphemeralCache)
Expand Down Expand Up @@ -772,18 +765,18 @@ Scev* ScalarEvolutionContext::AnalyzeNew(BasicBlock* block, GenTree* tree, int d
// "i = i + 1".
//
// Parameters:
// headerStore - Phi definition of the candidate primary induction variable
// headerPhi - Phi value of the candidate primary induction variable
// enterScev - SCEV describing start value of the primary induction variable
// stepDefBlock - Block containing the def of the step value
// stepDefData - Value of the def of the step value
//
// Returns:
// SCEV node if this is a simple addrec shape. Otherwise nullptr.
//
Scev* ScalarEvolutionContext::CreateSimpleAddRec(GenTreeLclVarCommon* headerStore,
ScevLocal* enterScev,
BasicBlock* stepDefBlock,
GenTree* stepDefData)
Scev* ScalarEvolutionContext::CreateSimpleAddRec(GenTreePhi* headerPhi,
ScevLocal* enterScev,
BasicBlock* stepDefBlock,
GenTree* stepDefData)
{
if (!stepDefData->OperIs(GT_ADD))
{
Expand All @@ -793,13 +786,34 @@ Scev* ScalarEvolutionContext::CreateSimpleAddRec(GenTreeLclVarCommon* headerStor
GenTree* stepTree;
GenTree* op1 = stepDefData->gtGetOp1();
GenTree* op2 = stepDefData->gtGetOp2();
if (op1->OperIs(GT_LCL_VAR) && (op1->AsLclVar()->GetLclNum() == headerStore->GetLclNum()) &&
(op1->AsLclVar()->GetSsaNum() == headerStore->GetSsaNum()))

auto getUseValue = [this](GenTree* value) -> GenTree* {
if (!value->OperIs(GT_LCL_VAR))
{
return nullptr;
}

GenTreeLclVarCommon* lcl = value->AsLclVarCommon();
if (!lcl->HasSsaName())
{
return nullptr;
}

LclVarDsc* lclDsc = m_comp->lvaGetDesc(lcl);
LclSsaVarDsc* ssaDsc = lclDsc->GetPerSsaData(lcl->GetSsaNum());
if (ssaDsc->GetDefNode() == nullptr)
{
return nullptr;
}

return ssaDsc->GetDefNode()->Data();
};

if (getUseValue(op1) == headerPhi)
{
stepTree = op2;
}
else if (op2->OperIs(GT_LCL_VAR) && (op2->AsLclVar()->GetLclNum() == headerStore->GetLclNum()) &&
(op2->AsLclVar()->GetSsaNum() == headerStore->GetSsaNum()))
else if (getUseValue(op2) == headerPhi)
{
stepTree = op1;
}
Expand Down
5 changes: 1 addition & 4 deletions src/coreclr/jit/scev.h
Original file line number Diff line number Diff line change
Expand Up @@ -230,10 +230,7 @@ class ScalarEvolutionContext

Scev* Analyze(BasicBlock* block, GenTree* tree, int depth);
Scev* AnalyzeNew(BasicBlock* block, GenTree* tree, int depth);
Scev* CreateSimpleAddRec(GenTreeLclVarCommon* headerStore,
ScevLocal* start,
BasicBlock* stepDefBlock,
GenTree* stepDefData);
Scev* CreateSimpleAddRec(GenTreePhi* headerPhi, ScevLocal* start, BasicBlock* stepDefBlock, GenTree* stepDefData);
Scev* MakeAddRecFromRecursiveScev(Scev* start, Scev* scev, Scev* recursiveScev);
Scev* CreateSimpleInvariantScev(GenTree* tree);
Scev* CreateScevForConstant(GenTreeIntConCommon* tree);
Expand Down

0 comments on commit 770df10

Please sign in to comment.