[PATCH] D13740: Catch combine opportunities for redundant imuls

[Sanjay Patel via llvm-commits]

spatel added inline comments.

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:2152-2154
@@ +2151,5 @@
+
+    SDValue AddNode = N0;
+    SDValue ConstNode = N1;
+    bool IsProfitable = true;
+
----------------
Please move this into a helper function or two. That should reduce the indentation, the parameters can take on the new variable names, and the high-level comments can apply to a whole function.

================
Comment at: test/CodeGen/X86/combine-multiplies.ll:1
@@ +1,2 @@
+; RUN: llc < %s -mtriple=i386-unknown-linux-gnu | FileCheck %s
+
----------------
I didn't step through the debug output, but I see that this optimization doesn't fire for x86-64. Did you look into that case?

================
Comment at: test/CodeGen/X86/combine-multiplies.ll:24
@@ +23,3 @@
+; CHECK: imull
+; CHECK-NOT: imull
+; CHECK: retl
----------------
I would prefer to see more of the correct output here rather than a CHECK-NOT. Eg:

CHECK:           imull	$400, %ecx, %edx        # imm = 0x190
CHECK-NEXT: leal	(%edx,%eax), %esi
CHECK-NEXT: movl	$11, 2020(%esi,%ecx,4)
CHECK-NEXT: movl	$22, 2080(%edx,%eax)
CHECK-NEXT: movl	$33, 10080(%edx,%eax)

You can commit this test case and check for the current codegen before proceeding with this patch; that will make the functional difference from the new combine clearer.

================
Comment at: test/CodeGen/X86/combine-multiplies.ll:42
@@ +41,3 @@
+
+attributes #0 = { nounwind "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="pentium4" "target-features"="+sse,+sse2" "unsafe-fp-math"="false" "use-soft-float"="false" }
+
----------------
Remove unnecessary attributes.

================
Comment at: test/CodeGen/X86/combine-multiplies.ll:44
@@ +43,3 @@
+
+; RUN: llc < %s -mtriple=i386-unknown-linux-gnu | FileCheck %s
+
----------------
Looks like the test case text was duplicated when you created the diff. 

Should there be a vector version of the test case? 


http://reviews.llvm.org/D13740