[PATCH] D15412: [SCEV][LAA] Add no overflow SCEV predicates and use use them to improve strided pointer detection

[Sanjoy Das via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added a comment.
This revision now requires changes to proceed.

(Reviewed the SCEV bits)


================
Comment at: include/llvm/Analysis/ScalarEvolution.h:274
@@ +273,3 @@
+  /// made on an AddRec expression. Given an affine AddRec expression
+  /// (a,+,b), we assume that it has nsw or nuw flags.
+  class SCEVAddRecOverflowPredicate final : public SCEVPredicate {
----------------
Nit: notation is `{a,+,b}`.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:287
@@ +286,3 @@
+    /// \brief Returns the AddRec expression that we've made assumptions for.
+    const SCEV *getExpr() const;
+
----------------
Nit: I'd return an `SCEVAddRecExpr *`.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9605
@@ +9604,3 @@
+    return S;
+  SCEVAddRecOverflowPredicate *OF = new (SCEVAllocator)
+      SCEVAddRecOverflowPredicate(ID.Intern(SCEVAllocator), AR, AddedFlags);
----------------
Minor nit: I'd use `auto *OF` here.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9675
@@ -9616,1 +9674,3 @@
 private:
+  bool addOverflowAssumption(const SCEV *S, SCEV::NoWrapFlags AddedFlags) {
+    auto *AR = static_cast<const SCEVAddRecExpr *>(S);
----------------
Please either directly pass in a `SCEVAddRecExpr` or use a `cast<>` instead of a `static_cast<>`.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9743
@@ +9742,3 @@
+
+const SCEV *SCEVAddRecOverflowPredicate::getExpr() const { return AR; }
+
----------------
Minor: I'd just put this definition in the header.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9746
@@ +9745,3 @@
+bool SCEVAddRecOverflowPredicate::implies(const SCEVPredicate *N) const {
+  const auto *Op = dyn_cast<const SCEVAddRecOverflowPredicate>(N);
+
----------------
I don't think you need to `dyn_cast<>` to `const T`.  If you're casting from a `const` pointer, you'll automatically get a `const` pointer.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:9871
@@ +9870,3 @@
+  const SCEV *Expr = getSCEV(V);
+  const auto *AR = static_cast<const SCEVAddRecExpr*>(Expr);
+  addPredicate(*SE.getAddRecOverflowPredicate(AR, Flags));
----------------
Please use `cast<SCEVAddRecExpr>`

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1977
@@ +1976,3 @@
+                                    bool Signed) {
+  Module *M = Loc->getParent()->getParent()->getParent();
+  IRBuilder<> OFBuilder(Loc);
----------------
There is an `Instruction::getModule`

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1999
@@ +1998,3 @@
+
+  if (SrcBits < DstBits) {
+    // We need to extend
----------------
I think you need to always zero extend the exit count.  For instance, if you have a loop with an exit count of `i8 255` then `i16 {0x7fff,+,1}` will sign overflow, even though `0x7fff + (1 * (-1))` will not sign overflow in either the `*` or the `+`.

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:2012
@@ +2011,3 @@
+  Value *Start = expandCodeFor(AR->getStart(), AR->getStart()->getType(), Loc);
+  Value *Stride = expandCodeFor(AR->getOperand(1),
+                                AR->getOperand(1)->getType(), Loc);
----------------
Please assert that `AR` is affine.  Also, it would be easier to read if you extracted out `const SCEV *Step = AR->getOperand(1)` and used `Step` here instead.

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:2034
@@ +2033,3 @@
+    if (Signed) {
+      APInt CmpMinValue = APInt::getSignedMinValue(DstBits).sext(SrcBits);
+      ConstantInt *CTMin = ConstantInt::get(M->getContext(), CmpMinValue);
----------------
I don't think this is needed -- the trip count is interpreted as an unsigned value, so you should just need to check that it is `ule iDstBits -1`.

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:2057
@@ +2056,3 @@
+  Value *OfMul = OFBuilder.CreateExtractValue(Mul, 1, "mul.overflow");
+  CallInst *Add = OFBuilder.CreateCall(AddF, {MulV, Start}, "uadd");
+  Value *OfAdd = OFBuilder.CreateExtractValue(Add, 1, "add.overflow");
----------------
Minor nit: please call this `"sadd"` or `"uadd"` depending on `AddF` (or just call it `"add"`).


http://reviews.llvm.org/D15412