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

[Sanjoy Das]

sanjoy added inline comments.

================
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
----------------
sanjoy wrote:
> 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.
On second thought, I take back what I said and agree with your reasoning -- I think the function is fine as is.


http://reviews.llvm.org/D11212