Page MenuHomePhabricator

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

Authored by dschuff on Oct 22 2014, 10:32 AM.

Details

Summary

Currently when emitting a label, a new data fragment is created for it if the
current fragment isn't a data fragment.
This change instead enqueues the label and attaches it to the next fragment
(e.g. created for the next instruction) if possible.

When bundle alignment is not enabled, this has no functionality change (it
just results in fewer extra fragments being created). For bundle alignment,
previously labels would point to the beginning of the bundle padding instead
of the beginning of the emitted instruction. This was not only less efficient
(e.g. jumping to the nops instead of past them) but also led to miscalculation
of the address of the GOT (since MC uses a label difference rather than
emitting a "." symbol).

Fixes https://code.google.com/p/nativeclient/issues/detail?id=3982

Diff Detail

Repository
rL LLVM

Event Timeline

dschuff updated this revision to Diff 15256.Oct 22 2014, 10:32 AM
dschuff retitled this revision from to [MC] Attach labels to existing fragments instead of using a separate fragment.
dschuff updated this object.
dschuff edited the test plan for this revision. (Show Details)
dschuff added reviewers: jvoung, eliben.
dschuff added a subscriber: Unknown Object (MLST).
jvoung added inline comments.Oct 22 2014, 11:57 AM
include/llvm/MC/MCObjectStreamer.h
50 ↗(On Diff #15256)

The function seems like it could be big in terms of code size, so this should probably not be defined inline.

73 ↗(On Diff #15256)

reset() should clear the PendingLabels vector too?

lib/MC/MCObjectStreamer.cpp
132 ↗(On Diff #15256)

nit: Can the "it." fit on this first line so that the whole sentence is on one line?

test/MC/X86/AlignedBundling/labeloffset.s
18 ↗(On Diff #15256)

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
28 ↗(On Diff #15256)

Should this one be Ltmp1, to test Ltmp1 as well? Otherwise it's not used.

39 ↗(On Diff #15256)

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?

60 ↗(On Diff #15256)

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.

64 ↗(On Diff #15256)

The code is careful to flush the pending labels if switching sections. Could you test for that too?

test/MC/X86/AlignedBundling/long-nop-pad.s
17 ↗(On Diff #15256)

Why did this end up changing from CHECK-NEXT to CHECK?

dschuff updated this revision to Diff 15277.Oct 22 2014, 1:56 PM
  • Review comments from jvoung
include/llvm/MC/MCObjectStreamer.h
50 ↗(On Diff #15256)

Done

73 ↗(On Diff #15256)

Done

lib/MC/MCObjectStreamer.cpp
132 ↗(On Diff #15256)

Done

test/MC/X86/AlignedBundling/labeloffset.s
18 ↗(On Diff #15256)

Sorry, emacs wanted tabs and I wanted spaces :)
Fixed.

28 ↗(On Diff #15256)

That was just to ensure that 2 adjacent labels didn't mess things up. I updated the test, see next comment.

39 ↗(On Diff #15256)

Yeah sorry I simplified the test from the my original one and that part got unintentionally oversimplified :) see if it makes more sense now.

60 ↗(On Diff #15256)

done.

64 ↗(On Diff #15256)

Done.

test/MC/X86/AlignedBundling/long-nop-pad.s
17 ↗(On Diff #15256)

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.

eliben edited edge metadata.Oct 22 2014, 2:22 PM

LGTM with some nits

lib/MC/MCObjectStreamer.cpp
45 ↗(On Diff #15277)

This comment probably belongs in the header?

153 ↗(On Diff #15277)

Can merge the dyn cast into the if here?

jvoung accepted this revision.Oct 22 2014, 2:38 PM
jvoung edited edge metadata.

LGTM too

This revision is now accepted and ready to land.Oct 22 2014, 2:38 PM
dschuff updated this revision to Diff 15282.Oct 22 2014, 3:47 PM
dschuff edited edge metadata.
  • Review from eliben
lib/MC/MCObjectStreamer.cpp
45 ↗(On Diff #15277)

Done

153 ↗(On Diff #15277)

Done.

dschuff closed this revision.Oct 22 2014, 3:48 PM
dschuff updated this revision to Diff 15283.

Closed by commit rL220439 (authored by @dschuff).