[PATCH] D52508: [InstCombine] Clean up after IndVarSimplify

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1197-1202
+  if (match(&I, m_c_BinOp(m_CombineAnd(m_Not(m_Value(Select)), m_Value(Not)),
+                          m_Value(A))) &&
+      (match(Select, m_Select(m_Value(Pred), m_Specific(A), m_Value(B))) ||
+       match(Select, m_Select(m_Value(Pred), m_Value(B), m_Specific(A)))) &&
+      IsFreeToInvert(B, B->hasOneUse()) && Select->hasOneUse() &&
+      Not->hasOneUse()) {
----------------
dmgreen wrote:
> lebedev.ri wrote:
> > This will most likely miss some complicated-and-hard-to-catch commutative cases.
> > First, you most certainly can use `m_OneUse()` matchers.
> > Second, those two extra matches will likely be a reason for misses.
> > I haven't tried it, please check *very* carefully, but i think this should be:
> > ```
> > auto my_Select =
> >     m_CombineOr(m_Select(m_Value(Pred), m_Specific(A), m_Value(B)),
> >                 m_Select(m_Value(Pred), m_Value(B), m_Specific(A)));
> > if (match(&I,
> >           m_c_BinOp(m_CombineAnd(m_OneUse(m_Not(m_CombineAnd(
> >                                      m_OneUse(my_Select), m_Value(Select)))),
> >                                  m_Value(Not)),
> >                     m_Value(A))) &&
> >     IsFreeToInvert(B, B->hasOneUse())) {
> > ```
> > Third, `IsFreeToInvert()` may still cause some misses, so maybe if you really want it should be a matcher too?
> > 
> Do you mean because we may match an add(not(X), not(select)) and miss the select? If so then A would be not(X) and we would have not(select(p, not(X), B)), with B being freely invertible and the whole thing would be inverted to select(p, X, ~B).
> 
> Or are there other cases I'm not seeing? 
> 
> I also ran into problems with what I think was using m_specific and m_value in the same pattern. We are maybe best off not using m_c_BinOp here?
> 
> Your point about the m_OneUse's is a good one, I'll use them to make this better.
> Or are there other cases I'm not seeing?

I don't know.
One thing i can say is that in general such a complicated all-on-one `match()`er
will miss less commutative cases than trying to manually match everything.

> I also ran into problems with what I think was using m_specific and m_value in the same pattern.
That is expected.
If you need to catch the value and then check for it in the same `match()`,
you use `m_Value()` as usual, and then use `m_Deferred()` instead of `m_Specific()`,
and it will just work.

> We are maybe best off not using m_c_BinOp here?
No.



https://reviews.llvm.org/D52508