[PATCH] D111643: [ValueTracking] Let propgatesPoison consider single poison operand.

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/include/llvm/Analysis/ValueTracking.h:603
+  ///
+  ///
   /// To filter out operands that raise UB on poison, you can use
----------------
nit: Too many new lines


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:5091
   if (const auto *I = dyn_cast<Instruction>(V)) {
-    if (propagatesPoison(cast<Operator>(I)))
-      return any_of(I->operands(), [=](const Value *Op) {
-        return directlyImpliesPoison(ValAssumedPoison, Op, Depth + 1);
-      });
+    if (isa<Operator>(I) && any_of(I->operands(), [=](const Use &Op) {
+          return propagatesPoison(Op) &&
----------------
The isa<> check is unnecessary -- all Instructions are Operators.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:5252
         auto *Opr = cast<Operator>(Cond);
-        if (propagatesPoison(Opr) && is_contained(Opr->operand_values(), V))
+        if (any_of(Opr->operands(), [V, Opr](const Use &U) {
+              return V == U && propagatesPoison(U);
----------------
Opr capture is unnecessary.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:5385
+  case Instruction::Select:
+    return PoisonOp == I->getOperand(0);
   case Instruction::Call:
----------------
I guess this is technically not wrong, but muddies the contract of the function a bit. I'd prefer using   `PoisonOp.getOperandNo() == 0` here.


================
Comment at: llvm/lib/Transforms/Instrumentation/PoisonChecking.cpp:303
         for (Value *V : I.operands())
           Checks.push_back(getPoisonFor(ValToPoison, V));
 
----------------
This doesn't look quite right to me, shouldn't this only push a check specifically for poison-propagating operands, rather than for all operands if at least one propagates?


================
Comment at: llvm/lib/Transforms/Scalar/IndVarSimplify.cpp:796
       for (const User *User : I->users())
         Worklist.push_back(cast<Instruction>(User));
   }
----------------
It would probably be more elegant to implement this the same as programUndefinedIfUndefOrPoison(), but I'm okay with this for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111643