[PATCH] D30446: [IndVars] Do not branch on poison

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy added inline comments.


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:2174
+    SmallVector<const Value *, 8> NotPoison;
+    propagateKnownNonPoison(BECond, NotPoison, InLoop);
+
----------------
reames wrote:
> Ok, seeing the usage here, I think this is probably the wrong interface for the problem even if we want to pursue this approach.  I think we'd be much better off with an analysis which provided an interface for asking whether a given Instruction is known to be non-poison.  Your IV analysis could be used, but you could also walk forward through uses to by-assumption non-poison use as well.  
I think a context-free analysis with a simpler interface will be less powerful (if I understand your suggestion correctly).  For instance, say we have:

```
loop:
  %iv = phi ...
  %iv.inc = add nsw i32 %iv, ...
  %cmp = %iv.inc s< ...
  call void @may_exit()
  br i1 %cmp, ...
```

An analysis can't correctly return true for `isKnownNonPoison(%iv.inc)` since it is possible for it to be poison and have `@may_exit` call `exit(0)` to avoid the UB due to branching on poison.

However, if we know the expression we're about to generate needs to be non-poison only at the point of the existing backedge, we can exploit the fact that the original condition for the branch is known to be not poison at that location, which is what I'm trying to do via `propagateKnownNonPoison`.


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:2193
+
+  if (PoisonGenerator) {
+    PoisonGenerator->dropPoisonGeneratingFlags();
----------------
reames wrote:
> minor: can't we do this before the more general search since we already have the information?  Or are you trying to check where the new IV itself is known non-poison?
I certainly want to first check if it is possible to get away without calling `dropPoisonGeneratingFlags` on anything at all.

However, I don't remember if there was a good reason to have `PoisonGenerator->dropPoisonGeneratingFlags()` before `IV->dropPoisonGeneratingFlags()`.  One (somewhat weak) reason to do this:  when I do `IV->dropPoisonGeneratingFlags()` I only affect stuff within the loop[0] I'm trying to optimize but when I do `PoisonGenerator->dropPoisonGeneratingFlags()` I may affect larger scope of things.

[0]: This is not really true (I affect stuff that uses the exit value of the IV, for one thing), which is why this line of reasoning is weak.


https://reviews.llvm.org/D30446