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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
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) {
----------------
JosephTremoulet wrote:
> spatel wrote:
> > 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 ).
> Yes, this same test reproduces an assertion[1] failure without this patch, but it does so nondeterministically -- I see it about every 20-30 runs.  The original compiland I was hitting this on would hit every ~2000 runs.  If I revert everything except the new assertion I added in isEqual, this test hits the new assertion something like 4 times out of 5 for me.
> 
> Happy for ideas how to trim it down / make it more deterministic.
> 
> [1] https://github.com/llvm/llvm-project/blob/590b1aee609d30346aee77a699381d21c538dd56/llvm/include/llvm/ADT/DenseMap.h#L404
It might make sense to write a unit-test that directly checks the result of hashing/equality on SimpleValue?  Granted, that would require exposing the SimpleValue API in a header somewhere, I think.

Alternatively, you could add a debugging flag to early-cse to make it do a linear scan over the hashtable entries after each failed hashtable lookup, to make sure it was supposed to fail.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62644