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

[Sanjay Patel via Phabricator via llvm-commits]

spatel 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))
----------------
Why match an arbitrary constant if this pattern requires a zero constant? Use m_Zero() or some variation of that.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:734
+  if (!match(CondVal, m_ICmp(Predicate, m_Value(CmpLHS), m_Constant(CmpRHS))) ||
+      (Predicate != ICmpInst::ICMP_EQ && Predicate != ICmpInst::ICMP_NE))
+    return nullptr;
----------------
Use ICmpInst::isEquality(Pred)


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:750
+    Mul = BinaryOperator::CreateMul(X, Builder.CreateFreeze(Y));
+  else
+    Mul = BinaryOperator::CreateMul(Builder.CreateFreeze(X), Y);
----------------
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.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108408