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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse updated this revision to Diff 147525.
jmorse added a comment.

Hi, Thanks all for reviewing,

> 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.

True, the patch only fixes a symptom,

> A couple paths forward i could see: [...]

I've taken a shot at the second option, which seems cleaner and reduces a reasonable amount of code duplication -- see updated patch. The downside is the more invasive patch, additional memory usage, and as I'm an LLVM newbie a risk that I've missed something (although the tests all pass on x86 Ubuntu 18.04).

Describing the logic of the revised patch:

- The opcode, main operands and flags get localised into SimpleValue
- During construction, the three replicated canonicalisations from the existing hash/equality functions are performed:
  - Commutative binary operations are ordered
  - Comparison operand swapping (the motivation for this patch)
  - Select pattern detection and operand ordering
- During hashing, the potentially canonicalised information is hashed in from the local storage (plus some non-canonicalised material for other instructions)
- Similarly for equality, if the instructions aren't already identical, we only examine the canonicalised local information

Which I think achieves the main aim: ensuring ordering decisions are only performed in one place (now the SimpleValue constructor). There's good coverage of these canonicalisation steps in test/Transforms/EarlyCSE/commute.ll.


https://reviews.llvm.org/D46866

Files:
  lib/Transforms/Scalar/EarlyCSE.cpp
  test/Transforms/EarlyCSE/sameoperand-cmp.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D46866.147525.patch
Type: text/x-patch
Size: 44138 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180518/4aa0e3c7/attachment.bin>