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

[Hyeongyu Kim via Phabricator via llvm-commits]

hyeongyukim marked 6 inline comments as done.
hyeongyukim added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3528
 
+  // Try to push freeze through instructions that propagate but don't produce
+  // poison as far as possible.  If operand of freeze follows three conditions
----------------
lebedev.ri wrote:
> This should go into a new function
I named it `PushFreezeToPreventPoisonFromPropagate,` is there a more appropriate name?
Please recommend me a better name!


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3543
+  if (auto *Inst = dyn_cast<Instruction>(Op0)) {
+    Value *NotGuaranteedNonPoisonOperand;
+    unsigned NumGuaranteedNonPoisonOperand = 0;
----------------
nikic wrote:
> lebedev.ri wrote:
> > lebedev.ri wrote:
> > > 
> > And actually, what do we want to do if there are several operands with the same `Value`?
> > Unless that is okay for us (we'd still only need a single `freeze`) and we want to rewrite them all,
> > i guess we need to record the operand index, not the value.
> This is why it should be `Use *` rather than `Value *`.
> 
> Just to be perfectly clear, this code should be `Use *NotGuaranteedNonPoisonOperand = nullptr` and then below `NotGuaranteedNonPoisonOperand = &U` and `replaceUse(*NotGuaranteedNonPoisonOperand, FI)`.
> 
> Also maybe rename `NotGuaranteedNonPoisonOperand` to `MaybePoisonOperand`, it's a mouthful :)
TBH I didn't consider the difference between `Use *` and `Value *`, but I realized it thanks to the comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105392