From ac9f3ea0b437d881bc9879601b0777139b9fd864 Mon Sep 17 00:00:00 2001 From: Sanjoy Das <sanjoy@playingwithpointers.com> Date: Fri, 14 Apr 2017 15:49:53 +0000 Subject: [PATCH] Remove NormalizeAutodetect; NFC It is cleaner to have a callback based system where the logic of whether an add recurrence is normalized or not lives on IVUsers. This is one step in a multi-step cleanup. llvm-svn: 300330 --- .../Analysis/ScalarEvolutionNormalization.h | 19 +-- llvm/lib/Analysis/IVUsers.cpp | 83 +++++++++-- llvm/lib/Analysis/ScalarEvolutionExpander.cpp | 4 +- .../Analysis/ScalarEvolutionNormalization.cpp | 131 +++--------------- .../Transforms/Scalar/LoopStrengthReduce.cpp | 12 +- 5 files changed, 104 insertions(+), 145 deletions(-) diff --git a/llvm/include/llvm/Analysis/ScalarEvolutionNormalization.h b/llvm/include/llvm/Analysis/ScalarEvolutionNormalization.h index 7c6423a21cfa..cc79aa9f60f3 100644 --- a/llvm/include/llvm/Analysis/ScalarEvolutionNormalization.h +++ b/llvm/include/llvm/Analysis/ScalarEvolutionNormalization.h @@ -40,21 +40,15 @@ namespace llvm { -class Instruction; -class DominatorTree; class Loop; class ScalarEvolution; class SCEV; -class Value; /// TransformKind - Different types of transformations that /// TransformForPostIncUse can do. enum TransformKind { /// Normalize - Normalize according to the given loops. Normalize, - /// NormalizeAutodetect - Detect post-inc opportunities on new expressions, - /// update the given loop set, and normalize. - NormalizeAutodetect, /// Denormalize - Perform the inverse transform on the expression with the /// given loop set. Denormalize @@ -63,16 +57,13 @@ enum TransformKind { /// PostIncLoopSet - A set of loops. typedef SmallPtrSet<const Loop *, 2> PostIncLoopSet; +typedef function_ref<bool(const SCEVAddRecExpr *)> NormalizePredTy; + /// TransformForPostIncUse - Transform the given expression according to the /// given transformation kind. -const SCEV *TransformForPostIncUse(TransformKind Kind, - const SCEV *S, - Instruction *User, - Value *OperandValToReplace, - PostIncLoopSet &Loops, - ScalarEvolution &SE, - DominatorTree &DT); - +const SCEV *TransformForPostIncUse(TransformKind Kind, const SCEV *S, + Optional<NormalizePredTy> Pred, + PostIncLoopSet &Loops, ScalarEvolution &SE); } #endif diff --git a/llvm/lib/Analysis/IVUsers.cpp b/llvm/lib/Analysis/IVUsers.cpp index 4b7d15b53649..de38cf8366c2 100644 --- a/llvm/lib/Analysis/IVUsers.cpp +++ b/llvm/lib/Analysis/IVUsers.cpp @@ -117,6 +117,50 @@ static bool isSimplifiedLoopNest(BasicBlock *BB, const DominatorTree *DT, return true; } +/// IVUseShouldUsePostIncValue - We have discovered a "User" of an IV expression +/// and now we need to decide whether the user should use the preinc or post-inc +/// value. If this user should use the post-inc version of the IV, return true. +/// +/// Choosing wrong here can break dominance properties (if we choose to use the +/// post-inc value when we cannot) or it can end up adding extra live-ranges to +/// the loop, resulting in reg-reg copies (if we use the pre-inc value when we +/// should use the post-inc value). +static bool IVUseShouldUsePostIncValue(Instruction *User, Value *Operand, + const Loop *L, DominatorTree *DT) { + // If the user is in the loop, use the preinc value. + if (L->contains(User)) + return false; + + BasicBlock *LatchBlock = L->getLoopLatch(); + if (!LatchBlock) + return false; + + // Ok, the user is outside of the loop. If it is dominated by the latch + // block, use the post-inc value. + if (DT->dominates(LatchBlock, User->getParent())) + return true; + + // There is one case we have to be careful of: PHI nodes. These little guys + // can live in blocks that are not dominated by the latch block, but (since + // their uses occur in the predecessor block, not the block the PHI lives in) + // should still use the post-inc value. Check for this case now. + PHINode *PN = dyn_cast<PHINode>(User); + if (!PN || !Operand) + return false; // not a phi, not dominated by latch block. + + // Look at all of the uses of Operand by the PHI node. If any use corresponds + // to a block that is not dominated by the latch block, give up and use the + // preincremented value. + for (unsigned i = 0, e = PN->getNumIncomingValues(); i != e; ++i) + if (PN->getIncomingValue(i) == Operand && + !DT->dominates(LatchBlock, PN->getIncomingBlock(i))) + return false; + + // Okay, all uses of Operand by PN are in predecessor blocks that really are + // dominated by the latch block. Use the post-incremented value. + return true; +} + /// AddUsersImpl - Inspect the specified instruction. If it is a /// reducible SCEV, recursively add its users to the IVUsesByStride set and /// return true. Otherwise, return false. @@ -207,19 +251,36 @@ bool IVUsers::AddUsersImpl(Instruction *I, // The regular return value here is discarded; instead of recording // it, we just recompute it when we need it. const SCEV *OriginalISE = ISE; - ISE = TransformForPostIncUse(NormalizeAutodetect, - ISE, User, I, - NewUse.PostIncLoops, - *SE, *DT); + + auto NormalizePred = [&](const SCEVAddRecExpr *AR) { + // We only allow affine AddRecs to be normalized, otherwise we would not + // be able to correctly denormalize. + // e.g. {1,+,3,+,2} == {-2,+,1,+,2} + {3,+,2} + // Normalized form: {-2,+,1,+,2} + // Denormalized form: {1,+,3,+,2} + // + // However, denormalization would use a different step expression than + // normalization (see getPostIncExpr), generating the wrong final + // expression: {-2,+,1,+,2} + {1,+,2} => {-1,+,3,+,2} + auto *L = AR->getLoop(); + bool Result = + AR->isAffine() && IVUseShouldUsePostIncValue(User, I, L, DT); + if (Result) + NewUse.PostIncLoops.insert(L); + return Result; + }; + + ISE = TransformForPostIncUse(Normalize, ISE, + Optional<NormalizePredTy>(NormalizePred), + NewUse.PostIncLoops, *SE); // PostIncNormalization effectively simplifies the expression under // pre-increment assumptions. Those assumptions (no wrapping) might not // hold for the post-inc value. Catch such cases by making sure the // transformation is invertible. if (OriginalISE != ISE) { - const SCEV *DenormalizedISE = - TransformForPostIncUse(Denormalize, ISE, User, I, - NewUse.PostIncLoops, *SE, *DT); + const SCEV *DenormalizedISE = TransformForPostIncUse( + Denormalize, ISE, None, NewUse.PostIncLoops, *SE); // If we normalized the expression, but denormalization doesn't give the // original one, discard this user. @@ -337,11 +398,9 @@ const SCEV *IVUsers::getReplacementExpr(const IVStrideUse &IU) const { /// getExpr - Return the expression for the use. const SCEV *IVUsers::getExpr(const IVStrideUse &IU) const { - return - TransformForPostIncUse(Normalize, getReplacementExpr(IU), - IU.getUser(), IU.getOperandValToReplace(), - const_cast<PostIncLoopSet &>(IU.getPostIncLoops()), - *SE, *DT); + return TransformForPostIncUse( + Normalize, getReplacementExpr(IU), None, + const_cast<PostIncLoopSet &>(IU.getPostIncLoops()), *SE); } static const SCEVAddRecExpr *findAddRecForLoop(const SCEV *S, const Loop *L) { diff --git a/llvm/lib/Analysis/ScalarEvolutionExpander.cpp b/llvm/lib/Analysis/ScalarEvolutionExpander.cpp index d15a7dbd20e6..66df34dbe7f3 100644 --- a/llvm/lib/Analysis/ScalarEvolutionExpander.cpp +++ b/llvm/lib/Analysis/ScalarEvolutionExpander.cpp @@ -1268,8 +1268,8 @@ Value *SCEVExpander::expandAddRecExprLiterally(const SCEVAddRecExpr *S) { if (PostIncLoops.count(L)) { PostIncLoopSet Loops; Loops.insert(L); - Normalized = cast<SCEVAddRecExpr>(TransformForPostIncUse( - Normalize, S, nullptr, nullptr, Loops, SE, SE.DT)); + Normalized = cast<SCEVAddRecExpr>( + TransformForPostIncUse(Normalize, S, None, Loops, SE)); } // Strip off any non-loop-dominating component from the addrec start. diff --git a/llvm/lib/Analysis/ScalarEvolutionNormalization.cpp b/llvm/lib/Analysis/ScalarEvolutionNormalization.cpp index 17d4c01a932d..becc4bb60102 100644 --- a/llvm/lib/Analysis/ScalarEvolutionNormalization.cpp +++ b/llvm/lib/Analysis/ScalarEvolutionNormalization.cpp @@ -12,88 +12,41 @@ // //===----------------------------------------------------------------------===// -#include "llvm/IR/Dominators.h" #include "llvm/Analysis/LoopInfo.h" #include "llvm/Analysis/ScalarEvolutionExpressions.h" #include "llvm/Analysis/ScalarEvolutionNormalization.h" using namespace llvm; -/// IVUseShouldUsePostIncValue - We have discovered a "User" of an IV expression -/// and now we need to decide whether the user should use the preinc or post-inc -/// value. If this user should use the post-inc version of the IV, return true. -/// -/// Choosing wrong here can break dominance properties (if we choose to use the -/// post-inc value when we cannot) or it can end up adding extra live-ranges to -/// the loop, resulting in reg-reg copies (if we use the pre-inc value when we -/// should use the post-inc value). -static bool IVUseShouldUsePostIncValue(Instruction *User, Value *Operand, - const Loop *L, DominatorTree *DT) { - // If the user is in the loop, use the preinc value. - if (L->contains(User)) return false; - - BasicBlock *LatchBlock = L->getLoopLatch(); - if (!LatchBlock) - return false; - - // Ok, the user is outside of the loop. If it is dominated by the latch - // block, use the post-inc value. - if (DT->dominates(LatchBlock, User->getParent())) - return true; - - // There is one case we have to be careful of: PHI nodes. These little guys - // can live in blocks that are not dominated by the latch block, but (since - // their uses occur in the predecessor block, not the block the PHI lives in) - // should still use the post-inc value. Check for this case now. - PHINode *PN = dyn_cast<PHINode>(User); - if (!PN || !Operand) return false; // not a phi, not dominated by latch block. - - // Look at all of the uses of Operand by the PHI node. If any use corresponds - // to a block that is not dominated by the latch block, give up and use the - // preincremented value. - for (unsigned i = 0, e = PN->getNumIncomingValues(); i != e; ++i) - if (PN->getIncomingValue(i) == Operand && - !DT->dominates(LatchBlock, PN->getIncomingBlock(i))) - return false; - - // Okay, all uses of Operand by PN are in predecessor blocks that really are - // dominated by the latch block. Use the post-incremented value. - return true; -} - namespace { /// Hold the state used during post-inc expression transformation, including a /// map of transformed expressions. class PostIncTransform { TransformKind Kind; + Optional<NormalizePredTy> Pred; PostIncLoopSet &Loops; ScalarEvolution &SE; - DominatorTree &DT; DenseMap<const SCEV*, const SCEV*> Transformed; public: - PostIncTransform(TransformKind kind, PostIncLoopSet &loops, - ScalarEvolution &se, DominatorTree &dt): - Kind(kind), Loops(loops), SE(se), DT(dt) {} + PostIncTransform(TransformKind kind, Optional<NormalizePredTy> Pred, + PostIncLoopSet &loops, ScalarEvolution &se) + : Kind(kind), Pred(Pred), Loops(loops), SE(se) {} - const SCEV *TransformSubExpr(const SCEV *S, Instruction *User, - Value *OperandValToReplace); + const SCEV *TransformSubExpr(const SCEV *S); protected: - const SCEV *TransformImpl(const SCEV *S, Instruction *User, - Value *OperandValToReplace); + const SCEV *TransformImpl(const SCEV *S); }; } // namespace /// Implement post-inc transformation for all valid expression types. -const SCEV *PostIncTransform:: -TransformImpl(const SCEV *S, Instruction *User, Value *OperandValToReplace) { - +const SCEV *PostIncTransform::TransformImpl(const SCEV *S) { if (const SCEVCastExpr *X = dyn_cast<SCEVCastExpr>(S)) { const SCEV *O = X->getOperand(); - const SCEV *N = TransformSubExpr(O, User, OperandValToReplace); + const SCEV *N = TransformSubExpr(O); if (O != N) switch (S->getSCEVType()) { case scZeroExtend: return SE.getZeroExtendExpr(N, S->getType()); @@ -108,44 +61,13 @@ TransformImpl(const SCEV *S, Instruction *User, Value *OperandValToReplace) { // An addrec. This is the interesting part. SmallVector<const SCEV *, 8> Operands; const Loop *L = AR->getLoop(); - // The addrec conceptually uses its operands at loop entry. - Instruction *LUser = &L->getHeader()->front(); - transform( - AR->operands(), std::back_inserter(Operands), - [&](const SCEV *Op) { return TransformSubExpr(Op, LUser, nullptr); }); + transform(AR->operands(), std::back_inserter(Operands), + [&](const SCEV *Op) { return TransformSubExpr(Op); }); // Conservatively use AnyWrap until/unless we need FlagNW. const SCEV *Result = SE.getAddRecExpr(Operands, L, SCEV::FlagAnyWrap); switch (Kind) { - case NormalizeAutodetect: - // Normalize this SCEV by subtracting the expression for the final step. - // We only allow affine AddRecs to be normalized, otherwise we would not - // be able to correctly denormalize. - // e.g. {1,+,3,+,2} == {-2,+,1,+,2} + {3,+,2} - // Normalized form: {-2,+,1,+,2} - // Denormalized form: {1,+,3,+,2} - // - // However, denormalization would use a different step expression than - // normalization (see getPostIncExpr), generating the wrong final - // expression: {-2,+,1,+,2} + {1,+,2} => {-1,+,3,+,2} - if (AR->isAffine() && - IVUseShouldUsePostIncValue(User, OperandValToReplace, L, &DT)) { - const SCEV *TransformedStep = - TransformSubExpr(AR->getStepRecurrence(SE), - User, OperandValToReplace); - Result = SE.getMinusSCEV(Result, TransformedStep); - Loops.insert(L); - } -#if 0 - // This assert is conceptually correct, but ScalarEvolution currently - // sometimes fails to canonicalize two equal SCEVs to exactly the same - // form. It's possibly a pessimization when this happens, but it isn't a - // correctness problem, so disable this assert for now. - assert(S == TransformSubExpr(Result, User, OperandValToReplace) && - "SCEV normalization is not invertible!"); -#endif - break; case Normalize: // We want to normalize step expression, because otherwise we might not be // able to denormalize to the original expression. @@ -161,10 +83,9 @@ TransformImpl(const SCEV *S, Instruction *User, Value *OperandValToReplace) { // (100 /u {1,+,1}<%bb16>)}<%bb25> // Note that the initial value changes after normalization + // denormalization, which isn't correct. - if (Loops.count(L)) { + if ((Pred && (*Pred)(AR)) || (!Pred && Loops.count(L))) { const SCEV *TransformedStep = - TransformSubExpr(AR->getStepRecurrence(SE), - User, OperandValToReplace); + TransformSubExpr(AR->getStepRecurrence(SE)); Result = SE.getMinusSCEV(Result, TransformedStep); } #if 0 @@ -178,8 +99,7 @@ TransformImpl(const SCEV *S, Instruction *User, Value *OperandValToReplace) { // stated above. if (Loops.count(L)) { const SCEV *TransformedStep = - TransformSubExpr(AR->getStepRecurrence(SE), - User, OperandValToReplace); + TransformSubExpr(AR->getStepRecurrence(SE)); Result = SE.getAddExpr(Result, TransformedStep); } break; @@ -194,7 +114,7 @@ TransformImpl(const SCEV *S, Instruction *User, Value *OperandValToReplace) { for (SCEVNAryExpr::op_iterator I = X->op_begin(), E = X->op_end(); I != E; ++I) { const SCEV *O = *I; - const SCEV *N = TransformSubExpr(O, User, OperandValToReplace); + const SCEV *N = TransformSubExpr(O); Changed |= N != O; Operands.push_back(N); } @@ -213,8 +133,8 @@ TransformImpl(const SCEV *S, Instruction *User, Value *OperandValToReplace) { if (const SCEVUDivExpr *X = dyn_cast<SCEVUDivExpr>(S)) { const SCEV *LO = X->getLHS(); const SCEV *RO = X->getRHS(); - const SCEV *LN = TransformSubExpr(LO, User, OperandValToReplace); - const SCEV *RN = TransformSubExpr(RO, User, OperandValToReplace); + const SCEV *LN = TransformSubExpr(LO); + const SCEV *RN = TransformSubExpr(RO); if (LO != LN || RO != RN) return SE.getUDivExpr(LN, RN); return S; @@ -225,9 +145,7 @@ TransformImpl(const SCEV *S, Instruction *User, Value *OperandValToReplace) { /// Manage recursive transformation across an expression DAG. Revisiting /// expressions would lead to exponential recursion. -const SCEV *PostIncTransform:: -TransformSubExpr(const SCEV *S, Instruction *User, Value *OperandValToReplace) { - +const SCEV *PostIncTransform::TransformSubExpr(const SCEV *S) { if (isa<SCEVConstant>(S) || isa<SCEVUnknown>(S)) return S; @@ -235,20 +153,17 @@ TransformSubExpr(const SCEV *S, Instruction *User, Value *OperandValToReplace) { if (Result) return Result; - Result = TransformImpl(S, User, OperandValToReplace); + Result = TransformImpl(S); Transformed[S] = Result; return Result; } /// Top level driver for transforming an expression DAG into its requested /// post-inc form (either "Normalized" or "Denormalized"). -const SCEV *llvm::TransformForPostIncUse(TransformKind Kind, - const SCEV *S, - Instruction *User, - Value *OperandValToReplace, +const SCEV *llvm::TransformForPostIncUse(TransformKind Kind, const SCEV *S, + Optional<NormalizePredTy> Pred, PostIncLoopSet &Loops, - ScalarEvolution &SE, - DominatorTree &DT) { - PostIncTransform Transform(Kind, Loops, SE, DT); - return Transform.TransformSubExpr(S, User, OperandValToReplace); + ScalarEvolution &SE) { + PostIncTransform Transform(Kind, Pred, Loops, SE); + return Transform.TransformSubExpr(S); } diff --git a/llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp b/llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp index 1dad080efbff..b584e00b9f4b 100644 --- a/llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp +++ b/llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp @@ -3160,8 +3160,7 @@ void LSRInstance::CollectFixupsAndInitialFormulae() { if (SE.isLoopInvariant(N, L) && isSafeToExpand(N, SE)) { // S is normalized, so normalize N before folding it into S // to keep the result normalized. - N = TransformForPostIncUse(Normalize, N, CI, nullptr, - TmpPostIncLoops, SE, DT); + N = TransformForPostIncUse(Normalize, N, None, TmpPostIncLoops, SE); Kind = LSRUse::ICmpZero; S = SE.getMinusSCEV(N, S); } @@ -4800,10 +4799,7 @@ Value *LSRInstance::Expand(const LSRUse &LU, // If we're expanding for a post-inc user, make the post-inc adjustment. PostIncLoopSet &Loops = const_cast<PostIncLoopSet &>(LF.PostIncLoops); - Reg = TransformForPostIncUse(Denormalize, Reg, - LF.UserInst, LF.OperandValToReplace, - Loops, SE, DT); - + Reg = TransformForPostIncUse(Denormalize, Reg, None, Loops, SE); Ops.push_back(SE.getUnknown(Rewriter.expandCodeFor(Reg, nullptr))); } @@ -4814,9 +4810,7 @@ Value *LSRInstance::Expand(const LSRUse &LU, // If we're expanding for a post-inc user, make the post-inc adjustment. PostIncLoopSet &Loops = const_cast<PostIncLoopSet &>(LF.PostIncLoops); - ScaledS = TransformForPostIncUse(Denormalize, ScaledS, - LF.UserInst, LF.OperandValToReplace, - Loops, SE, DT); + ScaledS = TransformForPostIncUse(Denormalize, ScaledS, None, Loops, SE); if (LU.Kind == LSRUse::ICmpZero) { // Expand ScaleReg as if it was part of the base regs.