[PATCH] D78615: [ValueTracking] Let propagatesPoison support binops/unaryops/cast/etc.

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/include/llvm/Analysis/ValueTracking.h:568
+  /// If I raises immediate UB (e.g. load poison), propagatesPoison returns
+  /// false.
   bool propagatesPoison(const Instruction *I);
----------------
aqjune wrote:
> nikic wrote:
> > As I mentioned on the other review, I don't think this is the best choice for this API. `propagatesPoison` should return `true` as much as possible for best results. It can return true for immediate UB, so imho it should. At least I don't see in which situation it would be advantageous to return false here.
> > 
> > For example, right now `propagatesPoison` on `udiv` returns false, because poison op2 results in IUB. However, poison op1 will propagate poison. Unless you want to distinguish which operand is poison, it would be better to always return true.
> > 
> > (Though distinguishing which operand is poison may be necessary for more accurate `select` handling, which propagates poison only on op1.)
> Okay, so it requires the users of propagatesPoison to specially deal with div/rem operations. We have `getGuaranteedNonPoisonOp`, which can be used to leave poison-propagating operands only. I think the concern makes sense.
I really don't think that having UB operations return true is the right semantic here.  It doesn't add any value.  If we have a UB operation with provable poison, then we've missed the opportunity to prune that path.  Having some later optimization trigger (which is what the proposed semantic does), feels like side stepping the problem.

(I'm just expressing an opinion, not blocking the patch.  I'm fine with this going in, we can continue the discussion and change our minds later if my view turns out to convince others.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78615/new/

https://reviews.llvm.org/D78615