[PATCH] D12905: [SCEV][LV] Introduce SCEV Predicates and use them to re-implement stride versioning
Adam Nemet via llvm-commits
llvm-commits at lists.llvm.org
Tue Sep 22 17:18:08 PDT 2015
anemet added a comment.
Hi Silviu,
Thanks for splitting this out, this is a much easier read now!
Adam
================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:295
@@ -292,1 +294,3 @@
+ /// The SCEV predicate containing all the SCEV-related assumptions.
+ SCEVPredicateSet &Pred;
};
----------------
I think I've commented on this too: this should be plural or have set in the name.
================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:545-576
@@ -539,28 +544,34 @@
+ /// The SCEV predicate containing all the SCEV-related assumptions.
+ std::unique_ptr<SCEVPredicateSet> Pred;
+
private:
/// \brief Analyze the loop. Substitute symbolic strides using Strides.
void analyzeLoop(const ValueToValueMap &Strides);
/// \brief Check if the structure of the loop allows it to be analyzed by this
/// pass.
bool canAnalyzeLoop();
void emitAnalysis(LoopAccessReport &Message);
/// We need to check that all of the pointers in this list are disjoint
/// at runtime.
RuntimePointerChecking PtrRtChecking;
/// \brief the Memory Dependence Checker which can determine the
/// loop-independent and loop-carried dependences between memory accesses.
MemoryDepChecker DepChecker;
Loop *TheLoop;
ScalarEvolution *SE;
const DataLayout &DL;
const TargetLibraryInfo *TLI;
AliasAnalysis *AA;
DominatorTree *DT;
LoopInfo *LI;
+ /// The SCEV predicate containing all the SCEV-related assumptions.
+ SCEVPredicateSet ScPredicates;
+
unsigned NumLoads;
----------------
I think I've already asked this before: why is the thing with unique_ptr needed?
================
Comment at: include/llvm/Analysis/ScalarEvolution.h:176
@@ -173,1 +175,3 @@
+ enum SCEVPredicateTypes { pSet, pEqual };
+
----------------
I think the enum tags are uppercase. Also this should probably be inside the class.
================
Comment at: include/llvm/Analysis/ScalarEvolution.h:190-196
@@ +189,9 @@
+ unsigned short getType() const { return SCEVPredicateType; }
+ /// Returns true if the predicate is always true. This means that no
+ /// assumptions were made and nothing needs to be checked at run-time.
+ virtual bool isAlwaysTrue() const = 0;
+ /// Return true if we consider this to be always false or if we've
+ /// given up on this set of assumptions (for example because of the
+ /// high cost of checking at run-time).
+ virtual bool isAlwaysFalse() const = 0;
+ /// Returns true if this predicate implies \p N.
----------------
Do these really ever happen aside for reaching the check threshold?
I think that we should just have an API to query the number of checks.
================
Comment at: include/llvm/Analysis/ScalarEvolution.h:1186-1189
@@ -1071,1 +1185,6 @@
+ /// Re-writes the SCEV according to the Predicates in \p Preds, by
+ /// applying overflow assumptions and sinking sext/zext expressions.
+ const SCEV *rewriteUsingPredicate(const SCEV *Scev, const Loop *L,
+ SCEVPredicateSet &A);
+
----------------
Stale comment. I also think that the function name should be plural.
================
Comment at: lib/Analysis/ScalarEvolution.cpp:115-119
@@ -111,1 +114,7 @@
+static cl::opt<unsigned>
+SCEVCheckThreshold("force-max-scev-checks", cl::init(16),
+ cl::Hidden,
+ cl::desc("Don't create SCEV predicates with more than "
+ "this number of assumptions."));
+
----------------
I don't think that this should be central threshold. It should be up to the client transformation to decide if the benefit of the transformation outweighs the overhead of the necessary checks.
================
Comment at: lib/Analysis/ScalarEvolution.cpp:8902-8909
@@ +8901,10 @@
+
+static Instruction *getFirstInst(Instruction *FirstInst, Value *V,
+ Instruction *Loc) {
+ if (FirstInst)
+ return FirstInst;
+ if (Instruction *I = dyn_cast<Instruction>(V))
+ return I->getParent() == Loc->getParent() ? I : nullptr;
+ return nullptr;
+}
+
----------------
Hmm, I think we're duplicating this now into the third file. Any better idea?
================
Comment at: lib/Analysis/ScalarEvolution.cpp:8911
@@ +8910,3 @@
+
+struct SCEVPredicateRewriter
+ : public SCEVVisitor<SCEVPredicateRewriter, const SCEV *> {
----------------
Should probably be a class since not all members are public.
Also it needs a comment.
This may be a silly question but why we need to override all these members?
================
Comment at: lib/Analysis/ScalarEvolution.cpp:9090
@@ +9089,3 @@
+
+ IRBuilder<> OFBuilder(Loc);
+ Instruction *FirstInst = nullptr;
----------------
OF for overflow? ;)
================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:318-321
@@ -317,2 +317,6 @@
+ // Adds code to check the overflow assumptions made by SCEV
+ std::pair<Instruction *, Instruction *>
+ addRuntimeOverflowChecks(Instruction *Loc);
+
/// Create an empty loop, based on the loop ranges of the old loop.
----------------
Overflow again.
http://reviews.llvm.org/D12905
More information about the llvm-commits
mailing list