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

[Juneyoung Lee via Phabricator via llvm-commits]

aqjune added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3538
+
+  // If OrigOp has multiple uses, multiple freeze will be inserted. To prevent a
+  // chain of freeze from being generated by a newly created freeze, we only
----------------
Maybe update this sentence as well? Insertion of multiple freezes isn't necessary, as discussed.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3563-3564
+
+  // We already folded "freeze (phi const, x)" to "phi const, (freeze x)"
+  assert(!isa<PHINode>(OrigOp));
+
----------------
hyeongyukim wrote:
> lebedev.ri wrote:
> > I do not expect that this assertion will hold, `foldOpIntoPhi()` has a number of bailouts.
> > 
> Do you mean `OrigOp` can be `PHINode` even if we already tried `foldOpIntoPhi()`?
> Then, is it correct to change the assertion to the below code?
> ```
> if (is<PHINode>(OrigOp))
>   return nullptr;
> ```
> 
I think so - +1 for `foldOpIntoPhi ` handling the freeze(phi) case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105392