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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

It looks like you uploaded a partial diff in the last iteration.



================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3535
+  // We already folded "freeze (phi const, x)" to "phi const, (freeze x)"
+  assert(!isa<PHINode>(OrigOp));
 
----------------
I'd move the assert next to the insertBefore call, as that's the only place where this matters.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3546
+  //   Op0.fr = Inst(Op1.fr, NonPoisonOps...)
+  //   Use(Op0.fr)          ; Op0.fr is already frozen. Wrong Transformation.
+  if (!OrigOpInst || !OrigOpInst->hasOneUse() ||
----------------
This doesn't look like a wrong transformation to me. Just a possibly non-profitable one, as we unnecessarily use a frozen value. (I agree that we shouldn't do this, at least not in this form, but the comment does not seem right.)


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3552
+  // If operand is guaranteed not to be poison, there is no need to add freeze
+  // to the operand. So we first find the operand that is not guranteed to be
+  // poison.
----------------
guranteed -> guaranteed


================
Comment at: llvm/test/Transforms/InstCombine/freeze.ll:121
   ret i1 %cond.fr
 }
----------------
It would be good to add a test for not pushing freeze through `add nuw %x, 1`. (In the future, we could push freeze through by dropping poison flags.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105392