[PATCH] D62644: [EarlyCSE] Ensure equal keys have the same hash value

[Sanjay Patel via Phabricator via llvm-commits]

spatel added subscribers: efriedma, craig.topper, arsenm, lebedev.ri.
spatel added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/EarlyCSE.cpp:335
+  // hash equality (DenseMap demands this as an invariant).
+  bool result = isEqualImpl(LHS, RHS);
+  assert(!result || (LHS.isSentinel() && LHS.Inst == RHS.Inst) ||
----------------
formatting: result -> Result


================
Comment at: llvm/test/Transforms/EarlyCSE/commute.ll:110-112
+; Min/max can also have an inverted predicate and select operands.
+
+define i1 @smin_inverted(i8 %a, i8 %b) {
----------------
For this and the related small tests, please commit them with baseline assertions as a preliminary patch, then rebase to show the improvements from this patch.


================
Comment at: llvm/test/Transforms/EarlyCSE/commute.ll:645-651
+; This test is a reproducer for a bug involving inverted min/max selects
+; hashing differently but comparing as equal.  It generates several equivalent
+; pairs with the intent that eventually the hash table will fill enough for
+; the probe paths of an equivalent pair to collide and the hash mismatch
+; to be noticed.  It also includes a negation of each negation to check for
+; the same issue one level deeper.
+define void @many_min_max(i32* %px, i32* %py, i32* %pout) {
----------------
How does this bug manifest? Can we cause an assertion failure without this patch?
cc'ing some more potential reviewers to see if anyone has ideas for a smaller than ~1000 line regression test to reproduce the bug. ( @lebedev.ri @efriedma @craig.topper @arsenm ).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62644