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

[Nikita Popov via Phabricator via llvm-commits]

nikic 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);
----------------
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.)


================
Comment at: llvm/lib/Transforms/Instrumentation/PoisonChecking.cpp:348
-     arguments, but LangRef doesn't state and propagatesPoison doesn't
-     include these two.
    - all binary ops w/vector operands - The likely interpretation would be that
----------------
jdoerfert wrote:
> I think this makes sense but we need to get input from others on this.
Lang ref does kind of state this by omission:

> Values other than phi nodes and select instructions depend on their operands.

We have select -> and/or transforms that are known to be unsound if and/or do not block poison, but work is underway to fix those.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78615