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

[Sanjoy Das]

sanjoy added inline comments.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:4144
@@ +4143,3 @@
+      if (isLoopInvariant(OtherOp, AddRec->getLoop()) &&
+          isGuaranteedToExecuteForEveryIteration(BinOp, AddRec->getLoop()))
+        return Flags;
----------------
broune wrote:
> sanjoy wrote:
> > I'm not sure that this is correct.  I think you need to prove that the instruction you used to justify UB if `V` was poison is what needs to execute on every loop iteration.
> > 
> > This is not a problem currently because `undefinedBehaviorIsGuaranteedIfFullPoison` only looks at a single basic block, but semantically, the following program will be problematic:
> > 
> > ```
> > 
> >  loop_header:
> >    %x = add nsw %p, %q
> >    ...
> > 
> >  outside_the_loop:
> >    ;; no other uses of %x
> >    store i32 42, GEP(@global, %x)
> > 
> > ```
> > 
> > You can conclude that the `%x` does not overflow in the last iteration, but that's it -- even if `%x` was poison in all the other iterations your program is well defined.
> As you point out, UB is not guaranteed on poison in this example, so even if `undefinedBehaviorIsGuaranteedIfFullPoison` is made stronger by considering more basic blocks, it should still return false here, right?
> 
> 
In that case why do we bother with `isGuaranteedToExecuteForEveryIteration`?

IOW, why not

```
  if (undefinedBehaviorIsGuaranteedIfFullPoison(BinOp))
    return Flags;

```


================
Comment at: lib/Analysis/ValueTracking.cpp:3327
@@ +3326,3 @@
+    !isa<InvokeInst>(I) && // might not terminate
+    !isa<ResumeInst>(I) && // has no successors
+    !isa<ReturnInst>(I); // has no successors
----------------
broune wrote:
> sanjoy wrote:
> > If you're dealing with terminator instructions here (I'm not sure that that is necessary) why is `br` okay?  Can't a `br` form an infinite loop?
> Yes, br can form an infinite loop. This function should still return false for br, as each single execution of br does terminate and then transfers execution to its successor (even if it is its own successor), but I suspect that that's not what you're getting at with this question. :)
> 
> Zooming out a bit, what I'm really using this function for is as a component of proving that one instruction B strongly post-dominates another instruction A. Part of that is to prove that there are no infinite paths from A to B, since in such a path we'd never actually get to B, even if B (non-strongly) post-dominates A. In the general case, yes, we'd also need to take into account loops between A and B within the same CFG/function and prove that they terminate. The main reason that I'm only considering one basic block at a time is to make things simpler by avoiding the need for that, since I think that this change is already complex enough as it is. Even then, we still need to look out for single instructions where a single invocation can be enough to prevent strong post-dominance (even within a basic block), and this function serves that purpose.
> 
> You're right that this over-all feature could work with a function like this that did not consider terminators. I included terminators anyway just because it gives this function a cohesive responsibility hat makes it easier to think about and it's also what would/will be needed when reasoning about strong post-dominance across basic blocks. I don't have any strong objection to taking the terminators out for now, though.
I think removing terminators and `assert(!isa<Terminator>(I) && "...");` will be an improvement.  If we later decide to make the algorithm more precise around control flow, then we can consider adding terminators.


http://reviews.llvm.org/D11212