[PATCH] D60723: [EarlyCSE] detect equivalence of selects with inverse conditions and commuted operands (PR41101)

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/EarlyCSE.cpp:311
+      return true;
+  }
+
----------------
This *looks* correct to me, but it seems like some pretty tricky code where it's easy to miss something. I'd suggest to add a helper to get rid of the not (which can be used both in the hashing and comparison code):

```
static bool matchSelect(Value *V, Value *&Cond, Value *&T, Value *&F) {
  if (match(V, m_Select(m_Value(Cond), m_Value(T), m_Value(F)))) {
    Value *CondNot;
    if (match(Cond, m_Not(m_Value(CondNot)))) {
      Cond = CondNot;
      std::swap(T, F);
    }
    return true;
  }
  return false;
}
```

And then only leave the handling of the comparison here:

```
Value *CondL, *CondR, *TL, *TR, *FL, *FR;
if (matchSelect(LHSI, CondL, TL, FL) && matchSelect(RHSI, CondR, TR, FR)) {
  if (TL == TR && FL == FR)
    return CondL == CondR;

  if (TL == FR && FL == TR) {
    CmpInst::Predicate PredL, PredR;
    Value *X, *Y;
    if (match(CondL, m_Cmp(PredL, m_Value(X), m_Value(Y))) &&
        match(CondR, m_Cmp(PredR, m_Specific(X), m_Specific(Y))) &&
        CmpInst::getInversePredicate(PredL) == PredR)
      return true;
  }
}
```

I think that would go a way towards making this more "obviously correct".


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

https://reviews.llvm.org/D60723