[PATCH] D108408: [InstCombine] Transform X == 0 ? 0 : X * Y --> X * freeze(Y).

[Filipp Zhinkin via Phabricator via llvm-commits]

fzhinkin added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:754
+  // Check that TrueVal is a constant instead of matching it with m_Zero()
+  // to handle the case when it is a scalar undef value.
+  auto *TrueValC = dyn_cast<Constant>(TrueVal);
----------------
spatel wrote:
> Should that be something like:
> "...when it is a scalar undef value or a vector containing non-zero elements that are masked by undef elements in the compare constant."
Yes, it looks better, thanks!


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:768
+
+  auto *FalseValI = cast<Instruction>(FalseVal);
+  auto *FrY = new FreezeInst(Y, Y->getName() + ".fr", FalseValI);
----------------
nikic wrote:
> I believe this may fail if the multiplication is a constant expression (we should bail in that case).
I was not able to reproduce that case w/ tests because all my const expressions were folded before invocation of newly added folding, but I'll add the check, thanks.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:769
+  auto *FalseValI = cast<Instruction>(FalseVal);
+  auto *FrY = new FreezeInst(Y, Y->getName() + ".fr", FalseValI);
+  IC.replaceOperand(*FalseValI, FalseValI->getOperand(0) == Y ? 0 : 1, FrY);
----------------
nikic wrote:
> It would be better to create the freeze using IRBuilder, because it will be queued for reprocessing that way. Or alternatively using `InsertNewInstBefore`, which also handles this correctly.
Thanks for suggestion, I'll use `InsertNewInstBefore`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108408