[PATCH] D11363: Allow merging of immediates within a basic block for code size savings and reduced footprint.

[Sanjay Patel]

spatel added a comment.

Hi Zia -

Nitpicks and a question.

Seems like a good optimization to me, but someone else should take a look and issue a verdict.


================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:287
@@ +286,3 @@
+    // Utility function to determine whether we should hoist common immediates
+    // out of instructions (within a BB) for code size, or not.
+    //
----------------
No comma after "size".

================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:289
@@ +288,3 @@
+    //
+    bool hoistImmediateForSize(SDNode *N) const {
+        uint32_t UseCount = 0;
----------------
Indentation (tab width) is off for this whole function.

================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:292
@@ +291,3 @@
+
+        // Don't want to hoist if we're not optimizing for size.
+        // Eventually, we'd like to remove this restriction.
----------------
Don't want to --> Do not

================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:293
@@ +292,3 @@
+        // Don't want to hoist if we're not optimizing for size.
+        // Eventually, we'd like to remove this restriction.
+        // See the comment in X86InstrInfo.td for more info.
----------------
Eventually, --> TODO:

================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:295
@@ +294,3 @@
+        // See the comment in X86InstrInfo.td for more info.
+        if (!OptForSize)
+            return false;
----------------
I realize this is intended to be a temporary check, but I don't know if this is the correct predicate (and I'm hoping someone else will answer this for us):
What about MinSize? There are lots of similar size checks sprinkled around, but AFAICT, if the function only has a MinSize attribute, we're missing all those cases. Should OptForSize imply MinSize?

================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:298
@@ +297,3 @@
+
+        // Walk all the users of the immediate
+        for (SDNode::use_iterator UI = N->use_begin(),
----------------
Period.

================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:304
@@ +303,3 @@
+
+            // This user is already selected. Count it as a legit use and
+            // move on.
----------------
legit --> legitimate

================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:327
@@ +326,3 @@
+        
+            // Immedates that are used for offsets as part of stack
+            // manipulation should be left alone. These are typically
----------------
Immedates --> Immediates

================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:329
@@ +328,3 @@
+            // manipulation should be left alone. These are typically
+            // used to indicate SP offsets for argument passing, and
+            // will get pulled into stores/pushes (implicitly).
----------------
No comma after "passing".

================
Comment at: lib/Target/X86/X86InstrArithmetic.td:618-626
@@ -617,11 +617,11 @@
 
 def Xi8  : X86TypeInfo<i8 , "b", GR8 , loadi8 , i8mem ,
-                       Imm8 , i8imm ,    imm,          i8imm   , invalid_node,
+                       Imm8 , i8imm ,    imm8_su,          i8imm   , invalid_node,
                        0, OpSizeFixed, 0>;
 def Xi16 : X86TypeInfo<i16, "w", GR16, loadi16, i16mem,
-                       Imm16, i16imm,    imm,          i16i8imm, i16immSExt8,
+                       Imm16, i16imm,    imm16_su,          i16i8imm, i16immSExt8_su,
                        1, OpSize16, 0>;
 def Xi32 : X86TypeInfo<i32, "l", GR32, loadi32, i32mem,
-                       Imm32, i32imm,    imm,          i32i8imm, i32immSExt8,
+                       Imm32, i32imm,    imm32_su,          i32i8imm, i32immSExt8_su,
                        1, OpSize32, 0>;
 def Xi64 : X86TypeInfo<i64, "q", GR64, loadi64, i64mem,
----------------
Weird spacing; over the 80-col edge.

================
Comment at: lib/Target/X86/X86InstrInfo.td:883
@@ +882,3 @@
+// TODO2 : This should really also be enabled under O2, but there's currently
+// an issues with RA where we don't pull the constants into their users
+// when we rematerialize them. I'll follow-up on enabling O2 after we fix that
----------------
issues --> issue

================
Comment at: lib/Target/X86/X86InstrInfo.td:887
@@ +886,3 @@
+// TODO3 : This is currently limited to single basic blocks (DAG creation
+// pulls block immediates to the top, and merges them if necessary).
+// Eventually, it would be nice to allow ConstantHoisting to merge constants
----------------
No comma after "top".

================
Comment at: test/CodeGen/X86/immediate_merging.ll:1
@@ +1,2 @@
+; RUN llc %s -o - | FileCheck %s
+target datalayout = "e-m:e-p:32:32-f64:32:64-f80:32-n8:16:32-S128"
----------------
This doesn't work for on my system. The "RUN" must be followed by a ":".

Also, it looks like most tests do a redirect rather than making the input file a param:
; RUN: llc -o - < %s

Don't know what the benefit is, but might as well stick to that pattern. :)

================
Comment at: test/CodeGen/X86/immediate_merging.ll:14
@@ +13,3 @@
+
+; test -Os to make sure immediates with multiple users don't get pulled in to
+; instructions.
----------------
test --> Test

================
Comment at: test/CodeGen/X86/immediate_merging.ll:55
@@ +54,3 @@
+
+; test -O2 to make sure that all immediates get pulled in to their users
+define i32 @foo2() {
----------------
test --> Test
Period at end.


http://reviews.llvm.org/D11363