[PATCH] D59442: Enable Operand Reordering for Commutative Instructions in the FunctionComparator/MergeFunctions

[JF Bastien via Phabricator via llvm-commits]

jfb added a subscriber: dexonsmith.
jfb added a comment.

I was talking to @dexonsmith about this review, and we like where this will eventually get LLVM. However, what really makes me uneasy is that I'm not sure the example you're trying to optimize happens often enough to warrant the algorithmic complexity in trying to perform the matching. Here are few more thoughts:

- In the limit, you're trying to prove that two function graphs are isomorphic and that's generally NP. We don't want that by default, though it might be interesting to go full NP at high optimization levels for a subset of problems that occur in real code (e.g. use some heuristic to determine when going NP might pay off and not take too long (whatever that means)).
- Without doing full graph isomorphism, there are probably some limited graph canonicalizations that would generally pay off on real-world code (for mergefuncs and LLVM in general). That sounds like what you tried to do in your paper on sequence alignment <https://www.lancaster.ac.uk/staff/wangz3/publications/cgo19.pdf>, and I am worried at that compile-time overhead of 15%.
- We want mergefuncs to help compile time, maybe even at `-O0`, so there needs to be a way to run mergefuncs early with very few complex matching rules.
- We'd like mergefuncs to be on by default before trying to make it super powerful. Solving the engineering problems that prevent it from being on by default is a good amount of work, but the payoff is that super powerful optimizations will get way more testing and real-world measurements (both on compile-time and size improvements) than if mergefuncs is off.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59442