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

[Juneyoung Lee via Phabricator via llvm-commits]

aqjune marked an inline comment as not done.
aqjune added inline comments.


================
Comment at: llvm/include/llvm/Analysis/ValueTracking.h:581
+  const Value *getGuaranteedNonUndefOrPoisonOp(const Instruction *I);
   const Value *getGuaranteedNonPoisonOp(const Instruction *I);
 
----------------
nikic wrote:
> As these functions are the same, I would just rename getGuaranteedNonPoisonOp to getGuaranteedNonUndefOrPoisonOp.
I think splitting these two is good (even if their contents are actually the same) because users like PoisonChecking want to only consider poison values. What do you think?


================
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;
----------------
nikic wrote:
> 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;`?
You're right, it shouldn't fall through. Will fix this


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


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4831
     }
 
+    if (!canCreateUndefOrPoison(Opr, PoisonOnly) &&
----------------
nikic wrote:
> 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.
Okay, will make a separate patch for this


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4840
+    if ((PoisonOnly || I->getType()->isIntegerTy(1)) &&
+        programUndefinedIfUndefOrPoison(I, PoisonOnly))
       return true;
----------------
nikic wrote:
> 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?
> 
> 
Yes, exactly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84242