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

Thu Jun 6 16:55:52 PDT 2019

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:
> efriedma wrote:
> > 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.
> I started coding up the debugging flag approach, but the hash table in question is a ScopedHashTable, which doesn't provide a way to enumerate the keys (unless I'm just missing it).  I'm not convinced that this test merits changing the ScopedHashTable API surface, but I'm happy to do it if there's reason to think there's other merit in adding key/value pair iteration to ScopedHashTable, or something -- thoughts?
> 
> I have similar reservations about exposing SimpleValue just for this test.  Do we have other examples of things exposed just for testing that I should follow?  E.g. would you expect it to be in EarlyCSE.h, or in some new EarlyCSE-private.h or similar?
> 
> I could also be convinced to remove or scale back this test.  I had two benefits in mind when I wrote it:
>   1) It reproduces an assertion failure on current sources
>   2) It validates that my changes don't just push the problem one level of looking-through-nots deeper
> 
> Regarding #1, if the hashes diverge again for the specific pattern this test repeats, then the selects in the other new tests above would not get CSE'd (except by super-rare hash collision), so the test would fail the CHECK lines there.  So we're protected against that particular regression regardless of this 1000-line test.  And the tests added above fail on current sources, from the same bug, just with a different symptom (missed optimization opportunity rather than assertion failure).
> 
> Regarding #2, I could just scale this down to one instance of the 11-line pattern, and add similar CHECK lines, and again have the coverage.
> 
> So I'm leaning toward making this test look a lot like the others, just with an extra "not" or two, but could be convinced to take one of the other approaches if someone cares.  Thoughts?
If we have some test coverage for the hashing, it isn't that important what we do here, I guess.  A debugging flag in EarlyCSE could be helpful to find other issues, but it's probably not worth writing a whole iteration framework for ScopedHashTables just for that.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62644