[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:738-741
+  auto *TrueValC = dyn_cast<Constant>(TrueVal);
+  if (TrueValC == nullptr ||
+      !match(FalseVal, m_c_Mul(m_Specific(X), m_Value(Y))))
+    return nullptr;
----------------
spatel wrote:
> fzhinkin wrote:
> > spatel wrote:
> > > 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.
> > > it would be good to pre-commit the tests 
> > 
> > Could you please explain what did you meant here?
> > 
> > As of other suggestions - I added comments to both code and tests and refactored multi-use test case to use @use_i32.
> > 
> > Thanks!
> > 
> > 
> > 
> I mean we can commit the tests with the current (not transformed) output verified by the CHECK lines. Then, we apply the code change here and update the tests. That way, we show the diffs on the tests rather than just the new output.
> 
> If you do not have commit access, let me know. I can push the baseline tests for you.
I don't have commit access, so I'll very appreciate if you commit the baseline tests. 
Besides changing checks I also removed tests' comments as comments does not make much sense without the transformation.

Here is the patch: {F18941958}. 

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108408