[PATCH] D46866: [EarlyCSE] Avoid a poorly defined instruction comparison

[Daniel Berlin via Phabricator via llvm-commits]

dberlin requested changes to this revision.
dberlin added a comment.

So, IMHO, the swapping should not be *determined* as part of the equality function, and that is your underlying problem.
This feels like a workaround instead of fixing that problem.

If you look at GVN and NewGVN, they take care of this the same way - the commutativeness/etc is taken care of by canonicalizing the expression prior to putting it in the hashtable.
The hashtable hashes and does equality based only on the contents of that structure as it exists in the table (IE they don't run around swapping operands in equality).

Admittedly, they have a different expression structure than EarlyCSE which makes this easier.

A couple paths forward i could see:

1. Determine what should be swapped or not swapped outside of hashing and equality functions, even if the equality function does the swapping.

IE add a flag to SimpleValue stating whether operands/predicate should be swapped, and that flag is set in the constructor.
Both hashing and equality do what that flag says prior to hashing/comparing.
Then you can make actual decisions about canonicalization outside of hashing and equality, even if hashing and equality have to do a little of the work.

This is kind of ugly but IMHO, still better than what we're doing now.

2. Create a CmpValue structure that works similar to how GVN/NewGVN (IE actually contains the operands and predicate).

All swapping/etc is taken care of by the things creating these structures, and hashing/equality of CmpValue is simply hashing of operands+predicate and equality comparison of structure members.


Repository:
  rL LLVM

https://reviews.llvm.org/D46866