[PATCH] D84242: [ValueTracking] Add UndefOrPoison/Poison-only version of relevant functions

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/include/llvm/Analysis/ValueTracking.h:581
+  const Value *getGuaranteedNonUndefOrPoisonOp(const Instruction *I);
   const Value *getGuaranteedNonPoisonOp(const Instruction *I);
 
----------------
As these functions are the same, I would just rename getGuaranteedNonPoisonOp to getGuaranteedNonUndefOrPoisonOp.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4792
   if (auto *C = dyn_cast<Constant>(V)) {
-    if (isa<UndefValue>(C))
+    if (!PoisonOnly && isa<UndefValue>(C))
       return false;
----------------
If PoisonOnly=true, does this actually end up returning true for UndefValue in all cases? It's not completely obvious to me. Maybe `if (isa<UndefValue>(C)) return PoisonOnly;`?


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4801
+      return (PoisonOnly || !C->containsUndefElement()) &&
+              !C->containsConstantExpression();
   }
----------------
As a followup, we may want to recursively check the elements instead.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4831
     }
 
+    if (!canCreateUndefOrPoison(Opr, PoisonOnly) &&
----------------
aqjune wrote:
> This is a fix to a bug that caused returning false more often. Since it was a small change, embedded here
I'd prefer to land this separately with the corresponding test case, otherwise it gets lost in the rest of the patch.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4840
+    if ((PoisonOnly || I->getType()->isIntegerTy(1)) &&
+        programUndefinedIfUndefOrPoison(I, PoisonOnly))
       return true;
----------------
To make sure I get the logic here: Undef is a bit-wise concept and programUndefinedIfUndefOrPoison() tells us whether we get UB for a //full// undef operand, while isGuaranteedNotToBeUndefOrPoison() tells us whether there can be //any// undef bits, so we can apply this only for the i1 case. Is that right?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84242