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

[Sanjay Patel]

================
Comment at: lib/CodeGen/MachineCombiner.cpp:214
@@ +213,3 @@
+/// and the new sequence has less instructions or the new sequence improves the
+/// critical path outright.
+/// The DAGCombine code sequence ends in MI (Machine Instruction) Root.
----------------
Gerolf wrote:
> remove outright
Fixed.

================
Comment at: lib/CodeGen/MachineCombiner.cpp:218
@@ -216,1 +217,3 @@
+/// sequence to replace the old sequence is that it cannot lengthen the critical
+/// path. This is decided by the formula:
 /// (NewRootDepth + NewRootLatency) <= (RootDepth + RootLatency + RootSlack)).
----------------
Gerolf wrote:
> This is no longer true. The equation could now be  '<'.
I read that statement as <= is still the minimum requirement, but let's see if I can make that clearer. I've added another explanatory statement after the formula to explain the role of the new parameter (NewCodeHasLessInsts). Let me know if you see a better way to word this. Thanks!

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:6292
@@ -6300,3 +6291,3 @@
 
 bool X86InstrInfo::hasPattern(MachineInstr &Root,
         SmallVectorImpl<MachineCombinerPattern::MC_PATTERN> &Pattern) const {
----------------
Gerolf wrote:
> This function does more than the name suggests. Also, I don't find it intuitive that is records two pattern. 
Looking at the AArch64 implementation, I thought it also could record 2 patterns per call. I agree that we should make this more obvious. Suggestions:
1. getPatterns()
2. getMachineCombinerPatterns()
3. getMachineCombinerPatternsForRootInst()

I opted for #2 in this revision of the patch. Since this change is just a naming difference but affects more files, we could make it a follow-on patch?

http://reviews.llvm.org/D10460

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