[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:733
+
+  if (!match(CondVal, m_ICmp(Predicate, m_Value(CmpLHS), m_Constant(CmpRHS))) ||
+      (Predicate != ICmpInst::ICMP_EQ && Predicate != ICmpInst::ICMP_NE))
----------------
spatel wrote:
> Why match an arbitrary constant if this pattern requires a zero constant? Use m_Zero() or some variation of that.
I'm capturing it to avoid bunch of casts to extract it from `CondVal` later on (to check if `TrueVal` merges with `CmpRHS` into zero). But as it rises some confusion I will use `m_Zero` here and will extract `CmpRHS` before actual use.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:750
+    Mul = BinaryOperator::CreateMul(X, Builder.CreateFreeze(Y));
+  else
+    Mul = BinaryOperator::CreateMul(Builder.CreateFreeze(X), Y);
----------------
spatel wrote:
> nikic wrote:
> > What ensures that the other case is `Y == CmpLHS`? Couldn't this be comparing a completely unrelated value?
> General note: each review comment like this one is a requirement for a regression test - either positive (ie, the transform works) or negative (the transform fails).
>  
> Instead of matching 2 arbitrary values with m_Mul(), consider using m_c_Mul(m_Specific(CmpLHS), m_Value(Y)).
> (Adjust the variable names and/or code comments, so they line up.)
@nikic You're right, thanks!

@spatel thanks for suggestion!


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:753
+  Mul->copyIRFlags(cast<Instruction>(FalseVal));
+  FalseVal->replaceAllUsesWith(Mul);
+  return Mul;
----------------
nikic wrote:
> nikic wrote:
> > In InstCombine, replaceInstUsesWith should be used in place of replaceAllUsesWith for worklist management reasons.
> Also, can you please test the case where the other user is before the select? I think as written you're inserting the mul before the select, so it may not dominate other users. You would have to adjust the insertion point.
> 
> (This might also be one of the rare cases where modifying the mul in place to freeze one operand is more elegant than creating a new one and replacing users...)
Thanks! Mul's modification requires less code in that case. Not sure about elegance, but I'll stick with it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108408