[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:185
+      }
+      return hash_combine(Inst->getOpcode(), Pred, TVal, FVal);
+    }
----------------
spatel wrote:
> nikic wrote:
> > Shouldn't the cmp operands be hashed as well?
> Good question. IIUC, this is not logically wrong, but using the cmp operands would improve the hashing (reduce chances of collision). Does that sound right?
That would be my understanding as well.


================
Comment at: llvm/lib/Transforms/Scalar/EarlyCSE.cpp:195
+      std::swap(TVal, FVal);
+    }
+    return hash_combine(Inst->getOpcode(), Cond, TVal, FVal);
----------------
I think this check should be moved above the cmp handling: You can have a select(not(cmp(pred x, y)), t, f) where canonicalization is prevented both on the select and the not due to multiple uses. In this case we'd presumably still want to treat it exactly the same way as a select(cmp(pred x, y), f, t), including the predicate canonicalization above.


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

https://reviews.llvm.org/D60723