[PATCH] [MC] Attach labels to existing fragments instead of using a separate fragment

Jan Voung jvoung at chromium.org
Wed Oct 22 11:57:09 PDT 2014


================
Comment at: include/llvm/MC/MCObjectStreamer.h:50
@@ +49,3 @@
+  void flushPendingLabels(MCFragment *F) {
+    if (PendingLabels.size()) {
+      if (!F) {
----------------
The function seems like it could be big in terms of code size, so this should probably not be defined inline.

================
Comment at: include/llvm/MC/MCObjectStreamer.h:73
@@ -53,3 +72,3 @@
   /// state management
   void reset() override;
 
----------------
reset() should clear the PendingLabels vector too?

================
Comment at: lib/MC/MCObjectStreamer.cpp:132
@@ +131,3 @@
+
+  // If there is a current fragment, mark the symbol as pointing into
+  // it. Otherwise queue the label and set its fragment pointer when we emit
----------------
nit: Can the "it." fit on this first line so that the whole sentence is on one line?

================
Comment at: test/MC/X86/AlignedBundling/labeloffset.s:18
@@ +17,3 @@
+# 26 bytes of instructions between the pop and the use of the pic base symbol.
+        movl    $3, 2(%ebx, %ebx)
+        movl    $3, 2(%ebx, %ebx)
----------------
nit: The indent of the real instructions are inconsistent.
* calll and popl are at one level
* movl and the bundle_lock are at a different level

================
Comment at: test/MC/X86/AlignedBundling/labeloffset.s:28
@@ +27,3 @@
+        cmpl    %eax, .Ltmp0
+        cmpl    %eax, (.Ltmp0-.L0$pb)
+        .bundle_unlock
----------------
Should this one be Ltmp1, to test Ltmp1 as well? Otherwise it's not used.

================
Comment at: test/MC/X86/AlignedBundling/labeloffset.s:39
@@ +38,3 @@
+
+# Also make sure it works with a non-relaxable instruction
+        .text
----------------
It's not clear to me what you mean here and perhaps I am missing context. The difference appears to be that in the first case "cmpl" is part of a bundle_lock/unlock group, but here it is not. I'm assuming that in the first case, instructions that are bundle_locked/unlocked simply assume the relaxed larger form, while in this second case, the instructions can sometimes use a small immediate?

================
Comment at: test/MC/X86/AlignedBundling/labeloffset.s:60
@@ +59,3 @@
+        cmpl    %eax, .Ltmp2
+# CHECK: cmpl %eax, 160
+        cmpl     %eax, (.Ltmp2-.L1$pb)
----------------
Small suggestion: maybe this test could have bar in a separate section (as if you compiled with ffunction-sections), then .Ltmp2 and the "80: popl" can be relative to the beginning of the function and not be sensitive to the size of the earlier functions in case they change.

================
Comment at: test/MC/X86/AlignedBundling/labeloffset.s:64
@@ +63,2 @@
+        popl    %ecx
+        jmp *%ecx
----------------
The code is careful to flush the pending labels if switching sections. Could you test for that too?

================
Comment at: test/MC/X86/AlignedBundling/long-nop-pad.s:17
@@ -16,3 +16,3 @@
 # CHECK-NEXT:   f:  nop
-# CHECK-NEXT:   1b: callq
+# CHECK:   1b: callq
 
----------------
Why did this end up changing from CHECK-NEXT to CHECK?

http://reviews.llvm.org/D5915






More information about the llvm-commits mailing list