[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:738-741
+  auto *TrueValC = dyn_cast<Constant>(TrueVal);
+  if (TrueValC == nullptr ||
+      !match(FalseVal, m_c_Mul(m_Specific(X), m_Value(Y))))
+    return nullptr;
----------------
lebedev.ri wrote:
> spatel wrote:
> > I'm still confused about this: we want to match a zero constant as the TrueVal based on the block comment for this function, so why not use m_Zero()?
> > Usually, we can match vectors that include undef elements, but we may not be able to propagate those undefs through a transform (typically, we would replace with a ConstantInt::getNullValue() to get a "full" zero).
> > 
> > If the partial undef logic is complicated, I think that would be ok to defer to a follow-up patch. Ie, let's get the basic/common case right, then look at corner cases if that really matters.
> Please read previous review comments in this patch. We are going in circles here.
Ah, sorry I missed that. And now I see that `@mul_select_eq_undef_vector` is testing a non-zero constant.
I think it would be good to pre-commit the tests and add some explanatory comments, so that's easier to spot.
Also, the multi-use test would be easier to follow if it took the more common `call void @use()` pattern.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108408