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

[Dave Green via Phabricator via llvm-commits]

dmgreen 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()) {
----------------
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.


https://reviews.llvm.org/D52508