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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3537
 
-      auto *MaybePoisonUse = MaybePoisonOperand.getValue();
-      auto *FrozenMaybePoisonOperand = new FreezeInst(MaybePoisonUse->get(), MaybePoisonUse->getUser()->getName() + ".fr");
+  if (!OrigOpInst->hasOneUse() ||
+      canCreateUndefOrPoison(dyn_cast<Operator>(OrigOp)))
----------------
I think it would be good to have a comment explaining why we don't do this iff the `OrigOpInst` has other uses.
We could just change them all to use `OrigFI` first. Why do we want to not do this?


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3544
+  // poison.
+  Use *MaybePoisonOperand;
+  for (Use &U : OrigOpInst->operands()) {
----------------
You probably want `= nullptr;`


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3554-3556
+  // If all operands are guaranteed to be non-poison, we can drop freeze.
+  if (!MaybePoisonOperand)
+    return OrigOp;
----------------
Note that it was a question, i don't know what should we be doing here.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3560
+      new FreezeInst(MaybePoisonOperand->get(),
+                     MaybePoisonOperand->getUser()->getName() + ".fr");
+
----------------
This looks like the freeze will get the name from it's user.
I would expect that it would get the base name from the non-frozen value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105392