[PATCH] D11212: [SCEV] Apply NSW and NUW flags via poison value analysis

[Sanjoy Das]

sanjoy added a comment.

First round of comments inline.  I'll do a more detailed review in the next couple of days.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4095
@@ -4090,2 +4094,3 @@
 }
 
+/// Returns true if I is guaranteed to be executed for every iteration of L.
----------------
I think `isGuaranteedToExecuteForEveryIteration`, `propagatesPoison`, `getOperandThatCausesUndefinedBehaviorIfPoison` and `undefinedBehaviorIsGuaranteedIfPoison` should live in `ValueTracking.h`, given that `ValueTracking` contains similar things like `isSafeToSpeculativelyExecute`.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4099
@@ +4098,3 @@
+                                                   const Loop *L) {
+  assert(I);
+  assert(L);
----------------
I don't think these asserts are adding much here.  Do you think it will be cleaner to take `I` and `L` by reference instead, to indicate this invariant?

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4114
@@ +4113,3 @@
+    // (non-strongly) post-dominate I.
+    if (dyn_cast<CallInst>(&LI) || LI.mayThrow())
+      return false;
----------------
Use `isa`.  Also `mayThrow` implies `isa<CallInst> || isa<ResumeInst>` and since `ResumeInst` is a terminator, I think this check can just be `isa<CallInst>` with a comment that this checks for both infinite loops and throws.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4116
@@ +4115,3 @@
+      return false;
+    if (&LI == I)
+      return true;
----------------
I'd swap this check with the previous one -- otherwise you'll always return `false` for calls.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4125
@@ +4124,3 @@
+///
+/// The exact rules for how poison propagates through instructions have not
+/// been settled as of 2015-07-10, so this function is conservative and only
----------------
I agree these are fairly "obvious", but I'd like to run these by David Majnemer and Nuno Lopes to make sure that these rules are at least congruent on what they have in mind for poison's future.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4130
@@ +4129,3 @@
+static bool propagatesPoison(const Instruction *I) {
+  assert(I);
+
----------------
I don't think this `assert` adds much.  It is fairly obvious that `I` is expected to be non null and if it is, we'll get a deterministic segfault in `I->getOpcode()`.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4210
@@ +4209,3 @@
+getOperandThatCausesUndefinedBehaviorIfPoison(const Instruction *I) {
+  assert(I);
+
----------------
Same comment as above about nullness of `I`.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4263
@@ +4262,3 @@
+    // and did not throw.
+    if ((isa<CallInst>(I) || I->mayThrow()) && I != PoisonI)
+      return false;
----------------
Some comment as in `isGuaranteedToExecuteForEveryIteration` for `CallInst` and `mayThrow`.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4272
@@ +4271,3 @@
+    if (YieldsPoison.count(I)) {
+      for (const Use &U : I->uses()) {
+        const Instruction *User = cast<Instruction>(U.getUser());
----------------
You can directly iterate over `I->users()`.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4284
@@ +4283,3 @@
+ScalarEvolution::getNoWrapFlagsFromPoison(const Value *V) {
+  assert(V);
+  const BinaryOperator *BinOp = cast<BinaryOperator>(V);
----------------
Same comment as earlier -- I don't think this `assert` adds much value.  You could take `Value &V` if your really cared about not passing in `nullptr`, but if I were you I'd just remove the `assert`.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4287
@@ +4286,3 @@
+
+  // Return early if there are no flags to propagae to the SCEV.
+  SCEV::NoWrapFlags Flags = SCEV::FlagAnyWrap;
----------------
Nit: propagate.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4307
@@ +4306,3 @@
+
+  if (!undefinedBehaviorIsGuaranteedIfPoison(BinOp))
+    return SCEV::FlagAnyWrap;
----------------
Question: why isn't `undefinedBehaviorIsGuaranteedIfPoison` enough?  IOW, why can't we have

```
  if (undefinedBehaviorIsGuaranteedIfPoison(BinOp))
    return Flags;
  else
    return SCEV::FlagAnyWrap;
```


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4312
@@ +4311,3 @@
+    const SCEV *Op = getSCEV(BinOp->getOperand(OpIndex));
+    if (auto AddRec = dyn_cast<SCEVAddRecExpr>(Op)) {
+      const int OtherOpIndex = 1 - OpIndex;
----------------
LLVM convention is `auto *AddRec`.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4372
@@ +4371,3 @@
+
+      if (auto OpSCEV = getExistingSCEV(Op)) {
+        AddOps.push_back(OpSCEV);
----------------
LLVM convention is `auto *`.


http://reviews.llvm.org/D11212