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

[Juneyoung Lee via Phabricator via llvm-commits]

aqjune marked 2 inline comments as done.
aqjune 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);
----------------
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.


================
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
----------------
nikic wrote:
> 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.
The sentence of LangRef implicitly describes the semantics of and/or, as @nikic says, but I have an impression that the semantics of and/or frequently has been controversial.

I remember that every possible other candidates for and/or was problematic because it breaks many optimizations on logical <-> arithmetic ops.

I'll bring concrete examples for these by running LLVM unit tests with modified semantics of and/or with Alive2 in a few days. After this issue is resolved, it will be great if we can explicitly state in LangRef why and/or should propagate poison regardless of masking values.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78615