[PATCH] D75808: [ValueTracking] isKnownNonZero, computeKnownBits for freeze

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/include/llvm/Analysis/ValueTracking.h:580
                                         const Instruction *CtxI = nullptr,
                                         const DominatorTree *DT = nullptr);
 
----------------
jdoerfert wrote:
> We should consider having 3 API functions now. isUndef, isPoison, isUndefOrPoison, with a common impl that takes two booleans. It makes it clearer at the call sites (IMHO).
I agree, this is a very confusing API.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:1800
     break;
+  case Instruction::Freeze:
+    if (isGuaranteedNotToBeUndefOrPoison(I->getOperand(0), true, Q.CxtI, Q.DT))
----------------
I don't think this is worth adding.  Such a freeze is trivial, and I'd expect the optimizer to simply remove it instead.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4550
   // TODO: Deal with other Constant subclasses.
-  if (isa<ConstantInt>(V) || isa<GlobalVariable>(V))
+  if (isa<ConstantInt>(V) || isa<ConstantFP>(V) || isa<GlobalVariable>(V) ||
+      isa<Function>(V))
----------------
Please test these additions through an existing user, separate and commit separately please.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4554
 
+  // Alloca instruction cannot be undef or poison.
+  if (isa<AllocaInst>(V))
----------------
Same here.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4561
+
+  if (auto BI = dyn_cast<BitCastInst>(V))
+    return isGuaranteedNotToBeUndefOrPoison(BI->getOperand(0), AllowUndef, CtxI,
----------------
We've ended up with potentially very deep recursion here.  In general, ValueTracking works with a small fixed depth window.  I think we need to add the same here.  (Should be done as a separate patch.)

Note: I don't think we can have a cycle in the graph since we only allow constant arguments to phis.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75808