From ed3113c26572452438fc2560cc31f7bfe8cd7cad Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 7 Oct 2024 16:42:57 +0200 Subject: [PATCH] JIT: Disallow SCEV analysis of non-values 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. --- src/coreclr/jit/inductionvariableopts.cpp | 4 +- src/coreclr/jit/scev.cpp | 62 ++++++++++++++--------- src/coreclr/jit/scev.h | 5 +- 3 files changed, 41 insertions(+), 30 deletions(-) diff --git a/src/coreclr/jit/inductionvariableopts.cpp b/src/coreclr/jit/inductionvariableopts.cpp index 2ea9cee6b9f21..5f0dcc4eecd52 100644 --- a/src/coreclr/jit/inductionvariableopts.cpp +++ b/src/coreclr/jit/inductionvariableopts.cpp @@ -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"); @@ -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"); diff --git a/src/coreclr/jit/scev.cpp b/src/coreclr/jit/scev.cpp index 8ae11130be681..a91a8f32ac58b 100644 --- a/src/coreclr/jit/scev.cpp +++ b/src/coreclr/jit/scev.cpp @@ -563,17 +563,10 @@ 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; @@ -581,7 +574,7 @@ Scev* ScalarEvolutionContext::AnalyzeNew(BasicBlock* block, GenTree* tree, int d // 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; @@ -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) @@ -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; @@ -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; @@ -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) @@ -772,7 +765,7 @@ 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 @@ -780,10 +773,10 @@ Scev* ScalarEvolutionContext::AnalyzeNew(BasicBlock* block, GenTree* tree, int d // 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)) { @@ -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; } diff --git a/src/coreclr/jit/scev.h b/src/coreclr/jit/scev.h index 654a450482086..6d55ccc80227c 100644 --- a/src/coreclr/jit/scev.h +++ b/src/coreclr/jit/scev.h @@ -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);