[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()) {
----------------
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?



https://reviews.llvm.org/D52508