This is an archive of the discontinued LLVM Phabricator instance.

[MC] Write padding into fragments when -mc-relax-all flag is used
ClosedPublic

Authored by phosek on Mar 4 2015, 9:02 PM.

Details

Summary

When instruction bundling is enabled and the -mc-relax-all flag is
set, we can write bundle padding directly into fragments and avoid
creating large number of fragments significantly reducing LLVM MC
memory usage.

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

Diff Detail

Event Timeline

phosek updated this revision to Diff 21251.Mar 4 2015, 9:02 PM
phosek retitled this revision from to [MC] Write padding into fragments when -mc-relax-all flag is used.
phosek updated this object.
phosek edited the test plan for this revision. (Show Details)
phosek added a reviewer: eliben.
phosek added a subscriber: mseaborn.
phosek updated this object.Mar 4 2015, 9:03 PM
phosek set the repository for this revision to rL LLVM.
phosek added a subscriber: Unknown Object (MLST).
eliben edited edge metadata.Mar 5 2015, 10:42 AM

Could you first update http://www.chromium.org/nativeclient/pnacl/aligned-bundling-support-in-llvm to explain the proposed scheme in more general terms?

eliben added inline comments.Mar 5 2015, 10:45 AM
include/llvm/MC/MCAsmLayout.h
112

Is there a good reason to keep this a member and not just a standalone function?

include/llvm/MC/MCAssembler.h
1252

Here and elsewhere - please document all the new functions/methods you add - explaining their arguments, return values, etc.

include/llvm/MC/MCObjectStreamer.h
86

What is FOffset for?

lib/MC/MCAssembler.cpp
649

Does this comment need updating?

tools/pnacl-llc/pnacl-llc.cpp
706 ↗(On Diff #21251)

This file doesn't see to belong here

phosek updated this revision to Diff 21532.Mar 9 2015, 5:29 PM
phosek edited edge metadata.

Addressing code review comments.

phosek added a comment.Mar 9 2015, 5:35 PM

I have added the description of the proposed scheme to http://www.chromium.org/nativeclient/pnacl/aligned-bundling-support-in-llvm as requested.

include/llvm/MC/MCAsmLayout.h
112

Refactored as a standalone function.

include/llvm/MC/MCAssembler.h
1252

Done for all the new functions/methods.

include/llvm/MC/MCObjectStreamer.h
86

I have update the comment to address that.

lib/MC/MCAssembler.cpp
649

I have update the comment to mention the optimization used when relax all flag is used.

tools/pnacl-llc/pnacl-llc.cpp
706 ↗(On Diff #21251)

Right, this is a leftover testing artifact; removed.

eliben added inline comments.Mar 11 2015, 8:39 AM
include/llvm/MC/MCAssembler.h
1252

Please place all method comments consistently on the method declaration, rather than definition

1261

Describe FOffest and FSize

include/llvm/MC/MCELFStreamer.h
117

Describe this new data structure

lib/MC/MCAssembler.cpp
657

writing

725

So this FIXME is potentially addressed?

lib/MC/MCELFStreamer.cpp
496

Each clause here had a comment explaining what's going on - please preserve that

568

Add a comment above this block explaining what it's doing

589

Add a comment above this block explaining what it's doing

phosek updated this revision to Diff 23084.Apr 1 2015, 12:14 PM

Addressing code review comments.

eliben accepted this revision.Apr 3 2015, 2:38 PM
eliben edited edge metadata.

LGTM

This revision is now accepted and ready to land.Apr 3 2015, 2:38 PM
This revision was automatically updated to reflect the committed changes.
mseaborn added inline comments.May 28 2015, 9:48 AM
llvm/trunk/lib/MC/MCObjectStreamer.cpp
148 ↗(On Diff #23662)

What is the reason for checking isBundlingEnabled() and getRelaxAll() here?

Could this explain why references to data sections are coming out wrong? (See the bug described here: https://chromiumcodereview.appspot.com/1129013004/#msg5)

You are calling flushPendingLabels() from mergeFragment(), but that doesn't get called if we are processing data sections. (mergeFragment() is called from EmitInstToData().)

When we are processing data directives such as ".long" or ".asciz", what would cause labels to get their offsets set (via setOffset())?