[PATCH] D60723: [EarlyCSE] detect equivalence of selects with inverse conditions and commuted operands (PR41101)
Nikita Popov via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Apr 16 00:47:25 PDT 2019
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
More information about the llvm-commits
mailing list