[PATCH] D105392: [InstCombine] Add optimization to prevent poison from being propagated.

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:5347
+  // If Operand is a constant, it is Well-defined.
+  for (auto operand = I->operands().begin(); operand != I->operands().end();
+       ++operand)
----------------
This change should no longer be needed.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3546
+    for (auto *operand = Inst->operands().begin();
+         operand != Inst->operands().end(); ++operand) {
+      if (isGuaranteedNotToBeUndefOrPoison(operand->get()))
----------------
xbolva00 wrote:
> Address clang tidy warnings? Plus above.
This should be `for (Use &U : Inst->operands())` or so.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3559
+        auto *FI = new FreezeInst(Op1, Op1->getName() + ".fr");
+        FI->insertAfter(Op1);
+        Op1->replaceUsesWithIf(
----------------
This is not generally safe. E.g. if `Op1` is an `invoke`, you can't insert after it. I think for now you should insert this before `Inst`.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3561
+        Op1->replaceUsesWithIf(
+            FI, [&](Use &U) { return !isa<FreezeInst>(U.getUser()); });
+        return replaceInstUsesWith(I, Op0);
----------------
What you're doing here is really a separate transform: You're canonicalizing from `v` to `freeze v`, if a freeze is present. This is presumably needed for your motivating case, but I think it would be better to handle it as an independent transform, as profitability is less obvious in this case. You also don't have any test coverage for this right now.

For now, I'd just do a `replaceUse()` for the single use.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105392