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

[Bjarke Hammersholt Roune]

broune added inline comments.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4099
@@ +4098,3 @@
+                                                   const Loop *L) {
+  assert(I);
+  assert(L);
----------------
sanjoy wrote:
> 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?
I would have preferred references, but all the other functions in this file take pointers, so I wanted to be consistent. Asserting non-null is itself not consistent with the rest of this file, so I took it out.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4114
@@ +4113,3 @@
+    // (non-strongly) post-dominate I.
+    if (dyn_cast<CallInst>(&LI) || LI.mayThrow())
+      return false;
----------------
sanjoy wrote:
> 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.
Done.

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

================
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
----------------
sanjoy wrote:
> 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.
Thank you. I'm curious what their thoughts are on it.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4130
@@ +4129,3 @@
+static bool propagatesPoison(const Instruction *I) {
+  assert(I);
+
----------------
sanjoy wrote:
> 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()`.
I removed it.

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

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

================
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());
----------------
sanjoy wrote:
> You can directly iterate over `I->users()`.
Thanks for the tip.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4284
@@ +4283,3 @@
+ScalarEvolution::getNoWrapFlagsFromPoison(const Value *V) {
+  assert(V);
+  const BinaryOperator *BinOp = cast<BinaryOperator>(V);
----------------
sanjoy wrote:
> 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`.
I removed 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;
----------------
sanjoy wrote:
> Nit: propagate.
Done.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4307
@@ +4306,3 @@
+
+  if (!undefinedBehaviorIsGuaranteedIfPoison(BinOp))
+    return SCEV::FlagAnyWrap;
----------------
sanjoy wrote:
> Question: why isn't `undefinedBehaviorIsGuaranteedIfPoison` enough?  IOW, why can't we have
> 
> ```
>   if (undefinedBehaviorIsGuaranteedIfPoison(BinOp))
>     return Flags;
>   else
>     return SCEV::FlagAnyWrap;
> ```
> 
I added a comment on why the other conditions are necessary. There may be a way around isLoopInvariant, but I'm not 100% sure about that, so I left it in.

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


http://reviews.llvm.org/D11212