This is an archive of the discontinued LLVM Phabricator instance.

Provide support for preserving assembly comments
ClosedPublic

Authored by niravd on May 6 2016, 8:16 AM.

Details

Summary

Preserve assembly comments from input in output assembly and flags to
toggle property. This is on by default for inline assembly and off in
llvm-mc.

Parsed comments are emitted immediately before an EOL which generally
places them on the expected line.

Diff Detail

Event Timeline

niravd updated this revision to Diff 56417.May 6 2016, 8:16 AM
niravd retitled this revision from to Provide support for preserving assembly comments.
niravd added reviewers: rtrieu, dwmw2.
niravd updated this object.
niravd added a subscriber: llvm-commits.
niravd updated this revision to Diff 57672.May 18 2016, 1:51 PM

Add llvm-mc test and deal with new LineComment structure

niravd updated this object.May 18 2016, 1:52 PM
niravd updated this revision to Diff 59583.Jun 3 2016, 11:05 AM

Transform preserved non-assembly comments to assembly-style comments

niravd added a reviewer: rnk.Jun 6 2016, 6:30 AM
majnemer edited edge metadata.Jun 8 2016, 11:16 AM

What is the rationale behind adding this functionality to MC?

include/llvm/MC/MCParser/MCAsmLexer.h
27–85 ↗(On Diff #59583)

Please do not reformat this enum in this differential.

What is the rationale behind adding this functionality to MC?

There are some projects which rely on emitted comments from inline assembly, e.g., Seabios. As a result when compiling with clang they turn off the integrated assembler.

niravd updated this revision to Diff 61252.Jun 20 2016, 7:31 AM
niravd edited edge metadata.

Resolve comments. Change C comment handling to generate better multi-line comments.

rnk edited edge metadata.Jul 6 2016, 8:18 AM

Hm, sorry, I never hit submit.

What is the rationale behind adding this functionality to MC?

There are some projects which rely on emitted comments from inline assembly, e.g., Seabios. As a result when compiling with clang they turn off the integrated assembler.

I think we should support this functionality so that we can give a reasonable workaround to the Linux kernel people for this: https://llvm.org/bugs/show_bug.cgi?id=18891

include/llvm/MC/MCStreamer.h
281

This is only called from MCAsmStreamer. Should we sink it down there and devirtualize it, or do we think there will be clients other than EmitEOL?

include/llvm/Target/TargetOptions.h
191 ↗(On Diff #59583)

This should really live in MCTargetOptions next to AsmVerbose, IMO.

lib/MC/MCAsmInfo.cpp
110

Why default to true?

niravd updated this revision to Diff 62955.Jul 6 2016, 1:29 PM
niravd edited edge metadata.

Update in light of rnk's comments

niravd added inline comments.Jul 7 2016, 1:07 PM
include/llvm/MC/MCStreamer.h
281

I'm inclined to keep it here with the other outputting functions. That said, by that argument it probably should be renamed emitExplicitComments.

include/llvm/Target/TargetOptions.h
191 ↗(On Diff #61252)

Make sense. Changed.

191 ↗(On Diff #61252)

Agreed

lib/MC/MCAsmInfo.cpp
110

Since the integrated assembler goes from preprocessor assembly to preprocessed assembly with some nominal cleanup, it seems like if we're assuming comments may have semantic value the default should be preservation for people who haven't reasoned about it explicitly.

rnk accepted this revision.Jul 7 2016, 1:13 PM
rnk edited edge metadata.

lgtm

lib/MC/MCAsmInfo.cpp
110

Also, I guess it's basically off by default if you emit an object file. OK.

This revision is now accepted and ready to land.Jul 7 2016, 1:13 PM
This revision was automatically updated to reflect the committed changes.