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

Sanjay Patel spatel at rotateright.com
Tue Jun 16 09:24:00 PDT 2015

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

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?



More information about the llvm-commits mailing list