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

[Sanjay Patel]

In http://reviews.llvm.org/D10460#190819, @Gerolf wrote:

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


It's certainly possible that we'll cause a compile-time spike with this patch (or even the existing code), but I would prefer to leave the safety harness as a follow-on patch pending some evidence that the case actually exists in the real world. Limiting this patch without that evidence seems like a premature compile-time optimization to me. The extra compile time should always be linear to the number of instructions, so it shouldn't explode too far on us.


================
Comment at: lib/Target/X86/X86InstrInfo.cpp:6284
@@ +6283,3 @@
+
+  MachineOperand Op11 = MI1->getOperand(1);
+  MachineOperand Op12 = MI1->getOperand(2);
----------------
Gerolf wrote:
> 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;
Good point. I took a slightly different approach to reduce even further!

================
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);
----------------
Gerolf wrote:
> 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?
reassociateOps() doesn't need any changes because the earlier patch assumed this change was coming; we made it (even the comments) assume the more general pattern could happen.

http://reviews.llvm.org/D10460

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