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

[Nuno Lopes via Phabricator via llvm-commits]

nlopes 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);
----------------
reames wrote:
> 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.)
I guess it's still useful to know that udiv propagates poison regardless whether we know it is UB or not.
Most of the times we won't know if an operation triggers UB or not, so it feels right to at least some information. We have other APIs to check whether an instruction triggers UB. You are right in that we are pushing a bit more responsibility to the user of this analysis (for perf reasons, not correctness), but for the reasons I've stated above, I think it's a good tradeoff.
I'm in favor of the patch as-is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78615