[PATCH] [x86] generalize reassociation optimization in machine combiner to 2 instructions

[Gerolf Hoflehner]

The code looks pretty good, but I'd like to understand better why the new code investigates more patterns. Also, the compile time increase to 5s looks huge. It is probably correct that it must be an outlier, however, is there anything that can be done to protect from a compile-time spike? On the other hand, the extra compile-time could be a good compile-time/performance trade-off. So one possibility I can think of is to  check the rate of success. For example: investigated N association patterns, never found a better code sequence (or perhaps some %threashold instead), so let's not waste more time on association patterns in this function. What do you think?
Thanks for clarifying the AARCH64 and MachineCombiner code!


================
Comment at: lib/Target/X86/X86InstrInfo.cpp:6284
@@ +6283,3 @@
+
+  MachineOperand Op11 = MI1->getOperand(1);
+  MachineOperand Op12 = MI1->getOperand(2);
----------------
There is a bit of code duplication you can avoid eg. by overloading hasVirtual...() and wrapping the code starting at MRI in a function. Then you would get something like if (hasVirtualRegDefsInBasicBlock(Op1,Op2, MBB) && Sibling=findAssocSibling(Op1,Op2,MBB, Commute) && hasVirtualRegDefsInBasicBlock(Sibling, MBB)) return true; return false;

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:6316
@@ +6315,3 @@
+    // machine combiner decide if changing the operands is worthwhile.
+    if (Commute) {
+      Patterns.push_back(MachineCombinerPattern::MC_REASSOC_AX_YB);
----------------
Allowing more than one pattern was part of the original design. What confused/confuses me is that in your old code you checked if operands had to be commuted in Root and Prev. But now the code only checks Root and potentially investigates two code sequences instead of one. Isn't that more expensive? And given that the order of the operands in Prev is not checked now, should there be a change in reassociateOps() addressing that?

http://reviews.llvm.org/D10460

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/