[PATCH] D77890: [ValueTracking] Implement isPoisonIf and canPushFreezeIntoOperands

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

I have enough questions about canCreatePoison() alone that I have not looked at impliesPoison() yet.
That might be a sign that we should split the review into 2 patches, but I think it's ok either way.



================
Comment at: llvm/include/llvm/Analysis/ValueTracking.h:596
+  /// Return true if I can create poison from non-poison operands.
+  /// If I is returning a vector value, true is returnd if it may contain
+  /// poison element when vectors without poison element are given.
----------------
typo: returnd


================
Comment at: llvm/include/llvm/Analysis/ValueTracking.h:597
+  /// If I is returning a vector value, true is returnd if it may contain
+  /// poison element when vectors without poison element are given.
+  bool canCreatePoison(const Instruction *I);
----------------
Add an example of the vector case in this code comment? Is it possible with the current code?


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4600-4602
+  case Instruction::Shl:
+  case Instruction::AShr:
+  case Instruction::LShr:
----------------
We could refine this, right? Ie, if the shift amount is a constant that is less than bitwidth, then shifts can not produce poison (unless they have wrapping/exact flags).
I don't know if that's the only case like this, so it's fine to make a TODO comment for a future patch if that's correct.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4619
+    // If index exceeds the length of the vector, it returns poison
+    auto *VTy = dyn_cast<VectorType>(I->getOperand(0)->getType());
+    unsigned IdxOp = I->getOpcode() == Instruction::InsertElement ? 2 : 1;
----------------
plain "cast" rather than "dyn_cast"?


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4643
+  // Return false if I is known not to create poison
+  switch (Opcode) {
+  case Instruction::PHI:
----------------
I find it confusing to switch on opcode, check flags, then switch on opcode again. Can we check flags first, and then switch on opcode just once?


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4646-4647
+  case Instruction::Select:
+  case Instruction::URem:
+  case Instruction::SRem:
+  case Instruction::ShuffleVector:
----------------
Immediate undefined behavior (eg, sdiv %x, 0) is explicitly not poison?
If correct, please note that in the documentation comment for this function, so there is no confusion.
(There's existing code called "MightCreatePoisonOrUB" in instcombine, but I'm not sure if it can be changed to use this new analysis function.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77890