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

[Derek Schuff]

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

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

================
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
----------------
jvoung wrote:
> nit: Can the "it." fit on this first line so that the whole sentence is on one line?
Done

================
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)
----------------
jvoung wrote:
> 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
Sorry, emacs wanted tabs and I wanted spaces :)
Fixed.

================
Comment at: test/MC/X86/AlignedBundling/labeloffset.s:28
@@ +27,3 @@
+        cmpl    %eax, .Ltmp0
+        cmpl    %eax, (.Ltmp0-.L0$pb)
+        .bundle_unlock
----------------
jvoung wrote:
> Should this one be Ltmp1, to test Ltmp1 as well? Otherwise it's not used.
That was just to ensure that 2 adjacent labels didn't mess things up. I updated the test, see next comment.

================
Comment at: test/MC/X86/AlignedBundling/labeloffset.s:39
@@ +38,3 @@
+
+# Also make sure it works with a non-relaxable instruction
+        .text
----------------
jvoung wrote:
> 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?
Yeah sorry I simplified the test from the my original one and that part got unintentionally oversimplified :) see if it makes more sense now.

================
Comment at: test/MC/X86/AlignedBundling/labeloffset.s:60
@@ +59,3 @@
+        cmpl    %eax, .Ltmp2
+# CHECK: cmpl %eax, 160
+        cmpl     %eax, (.Ltmp2-.L1$pb)
----------------
jvoung wrote:
> 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.
done.

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

================
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
 
----------------
jvoung wrote:
> Why did this end up changing from CHECK-NEXT to CHECK?
The label foo: now points to 1b the call instead of 0 the nop. so it gets printed in between the last nop and the call.

http://reviews.llvm.org/D5915