This is an archive of the discontinued LLVM Phabricator instance.

[COFF] Add cg_profile directive and .llvm.call-graph-profile section
ClosedPublic

Authored by zequanwu on Jun 12 2020, 4:26 PM.

Details

Summary

Add cg_profile directive and .llvm.call-graph-profile section in COFF.
The assembly format and binary are the same as ELF.

Diff Detail

Event Timeline

zequanwu created this revision.Jun 12 2020, 4:26 PM
zequanwu edited the summary of this revision. (Show Details)Jun 12 2020, 4:27 PM
zequanwu added inline comments.Jun 12 2020, 4:32 PM
llvm/lib/MC/MCParser/COFFAsmParser.cpp
336

Only store non-temporary symbols pairs.

In ELF, temporary symbols are also stored. I don't get the point of doing that. As I know, temporary symbols will not show up in object file. So, there is no reason to store temporary symbols.

llvm/tools/llvm-readobj/llvm-readobj.cpp
349–352

Probably, these two should be combined into one option --cg-profile.

MaskRay added inline comments.Jun 14 2020, 8:57 AM
llvm/lib/MC/MCParser/COFFAsmParser.cpp
307

If the code duplicates ELFAsmParser, please move it to AsmParser::

llvm/test/MC/AsmParser/directive_cgprofile.s
2

FileCheck %s (reuse CHECK:)

I'd expect a dedicated test for the new llvm-readobj dumping behaviour in "test/tools/llvm-readobj/COFF", probably using yaml as input if the format is supported or the binary content isn't ridiculous. We need to be careful not to create circular testing to ensure that llvm-readobj doesn't share the same bug as MC.

It probably makes sense to split the llvm-readobj dumping changes into a separate patch, that can be done separately, probably in addition to a third patch as mentioned inline for the option.

llvm/tools/llvm-readobj/COFFDumper.cpp
1974

I've not looked into whether this is possible, but if it is, it might be nice to share at least some of the COFF and ELF code, since presumably the section format is identical in both cases.

llvm/tools/llvm-readobj/llvm-readobj.cpp
349–352

Yeah, rather than adding a coff-specific option, I'd add --cg-profile and make --elf-cg-profile an alias for backwards compatibility (or possibly even delete it, but I'd be somewhat reluctant to do that). You can do that in a separate patch before this one.

Also, the new option, in whatever form it takes, needs adding to the CommandGuide doc for llvm-readobj and llvm-readelf (the latter only for the generic --cg-profile option, and only if applicable).

509

By using the new name, you can avoid this code change.

zequanwu updated this revision to Diff 270919.Jun 15 2020, 5:03 PM
zequanwu marked 2 inline comments as done.
  • Split llvm-readobj part to another patch.
  • Write .llvm-call-graph-profile section data in the same format as ELF.
zequanwu marked an inline comment as done.Jun 15 2020, 5:06 PM
zequanwu added inline comments.
llvm/lib/MC/MCParser/COFFAsmParser.cpp
307

Not exactly the same. See the comment below.

llvm/tools/llvm-readobj/COFFDumper.cpp
1974

COFF.h lack some functionalities to do the same parsing as ELF.h. I think this is also why the parsing of Addrsig section in COFF doesn't share any code with ELF, even if the section format is identical in both cases.

zequanwu retitled this revision from [COFF] Add .llvm.call-graph-profile and `--coff-cg-profile` dumpping to [COFF] Add .llvm.call-graph-profile section.Jun 15 2020, 5:09 PM
zequanwu retitled this revision from [COFF] Add .llvm.call-graph-profile section to [COFF] Add cg_profile directive and .llvm.call-graph-profile section.Jun 15 2020, 5:19 PM
zequanwu edited the summary of this revision. (Show Details)
zequanwu added a reviewer: hans.
MaskRay added inline comments.Jun 23 2020, 1:24 PM
llvm/lib/MC/MCParser/COFFAsmParser.cpp
336

Temporary symbols are a subcategory of local symbols. I think in ELF, it does not really matter whether a profile edge references a local symbol or not, a temporary symbol or not. A temporary symbol can also encode a profile edge.

Is there any particular reason you want to exclude temporary symbols from COFF?

llvm/lib/MC/MCWinCOFFStreamer.cpp
342

Why setIsWeakExternal?

zequanwu marked 2 inline comments as done.Jun 23 2020, 7:47 PM
zequanwu added inline comments.
llvm/lib/MC/MCParser/COFFAsmParser.cpp
336

From my understanding, since temporary symbols doesn't show up in final binary and the purpose of .llvm.call-graph-profile is to be used to optimize section layouts of final binary, storing temporary symbols doesn't help.

llvm/lib/MC/MCWinCOFFStreamer.cpp
342
MaskRay added inline comments.Jun 24 2020, 5:58 PM
llvm/lib/MC/MCParser/COFFAsmParser.cpp
336

Please drop isTemporary.

For temporary symbols (.L prefixed), the streamer can use a symbol representing the defined section. In ELF, this can cause creation of STT_SECTION. See D51047 for a COFF example.

llvm/lib/MC/MCWinCOFFStreamer.cpp
342

OK.

http://llvm.org/docs/Extensions.html#sht-llvm-call-graph-profile-section-call-graph-profile says that the undefined symbol is weak. It does not have to be specified that way...

zequanwu marked an inline comment as done.Jun 25 2020, 4:47 PM
zequanwu added inline comments.
llvm/lib/MC/MCParser/COFFAsmParser.cpp
336

Okay. I want to do so, but there is a problem.

https://reviews.llvm.org/D44965#1115454 says, temporary symbols should get null symbol index. And it is doing so by setting temporary symbol to Begin symbol of its section, https://github.com/llvm/llvm-project/blob/master/llvm/lib/MC/MCELFStreamer.cpp#L473, which will later update to null symbol index in computeSymbolTable (e.g. 4 in the test case). So, the .L.local temporary symbol will not show up when reading with llvm-readobj --cg-profile, https://github.com/llvm/llvm-project/blob/master/llvm/test/MC/ELF/cgprofile.s#L96.

In order to do the same, we need temporary symbols get null symbol indexes (or similar thing) in COFF. Is there a way to do that in COFF?

MaskRay added inline comments.Jun 25 2020, 6:05 PM
llvm/lib/MC/MCParser/COFFAsmParser.cpp
336

I don't know enough about COFF to suggest a solution, but I believe you can leave out temporary for now

  • don't exclude isTemporary() here
  • then you can move the implemention to the common AsmParser
  • ignore isTemporary() symbols in MCWinCOFFStreamer::emitCGProfileEntry
zequanwu updated this revision to Diff 273586.Jun 25 2020, 8:52 PM
  • Move ParseDirectiveCGProfile to MCAsmParserExtension.
  • Ignore temporary symbols in MCWinCOFFStreamer::emitCGProfileEntry.

Friendly ping.

MaskRay accepted this revision.Jul 10 2020, 3:34 PM

LGTM with one nit.

llvm/test/MC/COFF/cgprofile.s
117

No newline

This revision is now accepted and ready to land.Jul 10 2020, 3:34 PM
zequanwu updated this revision to Diff 277167.Jul 10 2020, 4:40 PM

rebase and add newline at end of file.

zequanwu closed this revision.EditedJul 10 2020, 5:37 PM

This was landed, but I accidentally wrote wrong diff ID (D83597) in the commit message: https://github.com/llvm/llvm-project/commit/0f0c5af3db9b0159d9b1a89faff3bd047510b628