Page MenuHomePhabricator

[ELF] Dump symbols ordered by profiled guided section layout to file.
ClosedPublic

Authored by tcwang on Mar 13 2019, 10:42 AM.

Details

Summary

In D36351, Call-Chain Clustering (C3) heuristic is implemented with
option --call-graph-ordering-file <file>.
This patch adds a flag --print-symbol-order=<file> to LLD, and when specified,
it prints out the symbols ordered by the heuristics to the file.
The symbols printout is helpful to those who want to understand the
heuristics and want to reproduce the ordering with --symbol-ordering-file
in later pass.

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ruiu added inline comments.Mar 13 2019, 11:04 AM
lld/ELF/CallGraphSort.cpp
230 ↗(On Diff #190449)

This is obvious, no need to explain by comment.

231 ↗(On Diff #190449)

This looks too fancy compared to other messages such as one in MapFile.cpp. Let's make it less fancy.

I'd make the first line "Symbols ordered by call-chain clustering:\n", followed by a list of symbols, terminated by a blank line.

236 ↗(On Diff #190449)

Are you sure that all input sections have a name?

lld/test/ELF/cgprofile-print.s
8 ↗(On Diff #190449)

This does not seem to test the feature that you want to use. Don't you want to print out the sorting order as a result of parsing .llvm.call-graph-profile sections, do you?

tcwang updated this revision to Diff 190457.Mar 13 2019, 11:24 AM
tcwang marked 4 inline comments as done.

Fixing the comments.

Thank you so much for the comments! Do you care to elaborate more on the test? I am not sure what I should test? Thanks!

lld/ELF/CallGraphSort.cpp
236 ↗(On Diff #190449)

Not sure... But if the section doesn't, should I print an empty line, or just skip it?

lld/test/ELF/cgprofile-print.s
8 ↗(On Diff #190449)

I am sorry that I think I am not sure I understand the comment completely. I was trying to add --verbose to LLD while passing in a call graph file and check the standard print with the expected ordering. (And I have removed the checks in the object file, since this check should be done in another test already.) Should I test something else instead?

tcwang marked an inline comment as not done.Mar 13 2019, 11:24 AM
tcwang marked an inline comment as not done.
pcc added inline comments.
lld/ELF/CallGraphSort.cpp
235 ↗(On Diff #190457)

Should this go to a file with a user-specified name (without the header) instead of stdout? I'd expect the output of this to be consumed by a script, and sending it to a file would probably make it easier for said script to consume.

Also, should this be listing the symbols instead of the sections if the intent is to pass this to --symbol-ordering-file?

ruiu added inline comments.Mar 13 2019, 11:34 AM
lld/ELF/CallGraphSort.cpp
236 ↗(On Diff #190449)

I don't think printing out a blank line is useful, but what I wanted to ask is if it is ever possible to be unnamed. We shouldn't guard it against an empty name "just in case".

lld/test/ELF/cgprofile-print.s
8 ↗(On Diff #190449)

I thought that you wanted to print a result of a compiler-generated callgraph data stored into .llvm.call-graph-profile sections (there's such feature in lld and LLVM). If that's not the case, never mind.

tcwang updated this revision to Diff 190465.Mar 13 2019, 11:54 AM
tcwang marked an inline comment as done.

Adding check against empty InputSection names, and don't print out empty names.

Thanks again for the comments!

lld/ELF/CallGraphSort.cpp
235 ↗(On Diff #190457)

I strongly agree that these suggestions would improve our lives if we want to use the outputs to feed --symbol-ordering-file in later pass. (Actually, right now I have to use a separate script to process the standard output and removing the headers to generate order file.)

I'd love to improve this part, if it is OK to add a flag to support it. (And do you have any suggestions on the name of the flag?) Also, do you have some suggestions how to remove the headers cleanly? I don't see there's an option to remove the added prefix in the Name field of InputSectionBase? I don't think I should just try to remove ".text" (or ".text.hot" in some cases). Thank you!

236 ↗(On Diff #190449)

I do see empty Name when testing against some programs. Maybe we should check empty Name before printing to avoid empty lines?

lld/test/ELF/cgprofile-print.s
8 ↗(On Diff #190449)

I think this option --call-graph-ordering-file is consuming the call graph and directly generate a map of sections with order. I think I only need to print out the ordering in the map for this purpose. So that's why I test it against.

ruiu added inline comments.Mar 13 2019, 12:57 PM
lld/ELF/CallGraphSort.cpp
235 ↗(On Diff #190457)

OK, that's fine. If that's the case, print out only symbol names to the file (without the header line). As to the option name, --print-symbol-order=FILENAME?

tcwang updated this revision to Diff 190525.Mar 13 2019, 3:56 PM

Add a flag to save the output. Also printing out the symbol names of the ordering, instead of
the section names.

tcwang marked an inline comment as done.Mar 13 2019, 3:56 PM
tcwang added inline comments.
lld/ELF/CallGraphSort.cpp
235 ↗(On Diff #190457)

Thanks!

tcwang updated this revision to Diff 190526.Mar 13 2019, 3:58 PM

Update title.

tcwang retitled this revision from [ELF] Print symbols ordered by profiled guided section layout with verbose. to [ELF] Dump symbols ordered by profiled guided section layout to file..Mar 13 2019, 3:59 PM
tcwang edited the summary of this revision. (Show Details)
ruiu added inline comments.Mar 13 2019, 4:02 PM
lld/ELF/CallGraphSort.cpp
243 ↗(On Diff #190525)

We do not use auto unless its type is obvious from right-hand side.

auto* Foo -> auto *Foo

246–247 ↗(On Diff #190525)

You don't need to cast -- I think you can directly compare Sections[SecIndex] == D->Section.

lld/ELF/Driver.cpp
831 ↗(On Diff #190525)

Config members has the same name as their corresponding command line options, so please name it PrintSymbolOrder.

tcwang updated this revision to Diff 190532.Mar 13 2019, 4:13 PM
tcwang marked 2 inline comments as done.

Fix comments. Add one more test to test the outputs of the flag can be used to
reproduce the order with --symbol-ordering-file

Thanks!

lld/ELF/CallGraphSort.cpp
243 ↗(On Diff #190525)

Ok. I will not use auto in this case. Thanks!

lld/ELF/Driver.cpp
831 ↗(On Diff #190525)

Ok. I'll change the name.

ruiu added inline comments.Mar 13 2019, 4:20 PM
lld/ELF/CallGraphSort.cpp
243 ↗(On Diff #190525)

Let's avoid using the same name for a variable and a class. I believe you have shadowed class Symbol by a variable Symbol. That's confusing. A common variable name is Sym.

tcwang updated this revision to Diff 190538.Mar 13 2019, 4:23 PM

Fix comments

tcwang marked an inline comment as done.Mar 13 2019, 4:24 PM
tcwang added inline comments.
lld/ELF/CallGraphSort.cpp
243 ↗(On Diff #190525)

Oh, right. I'll fix it.

MaskRay added inline comments.Mar 13 2019, 11:37 PM
lld/ELF/Options.td
278 ↗(On Diff #190538)

--call-graph-ordering file

tcwang updated this revision to Diff 190644.Mar 14 2019, 9:04 AM

Fix comments

tcwang marked 2 inline comments as done.Mar 14 2019, 9:04 AM
tcwang added inline comments.
lld/ELF/Options.td
278 ↗(On Diff #190538)

Thanks!

grimar added a subscriber: grimar.Mar 17 2019, 7:02 AM
tcwang marked 15 inline comments as done.Mar 21 2019, 9:06 AM

Ping. I think I have addressed all the comments here. Please let me know if there are more concerns to address. Thanks!

tcwang edited the summary of this revision. (Show Details)Mar 21 2019, 2:16 PM
ruiu added inline comments.Mar 21 2019, 2:59 PM
lld/ELF/CallGraphSort.cpp
244 ↗(On Diff #190644)

So I think that checking name is fragile. What do you actually want to skip by doing this?

lld/ELF/Driver.cpp
1477 ↗(On Diff #190644)

80 cols.

lld/ELF/Options.td
278 ↗(On Diff #190644)

This help message seem a little bit foreign compared to other help messages. A more natural help message would be probably something this:

Print a symbol order specified by --call-graph-ordering-file into to the specified file

Be careful not to put a full stop at the end of a help message.

lld/test/ELF/cgprofile-print.s
35–37 ↗(On Diff #190644)

Remove trailing extra newlines.

tcwang updated this revision to Diff 191791.Mar 21 2019, 3:27 PM
tcwang marked 3 inline comments as done.

Fix comments

tcwang marked an inline comment as not done.Mar 21 2019, 3:28 PM
tcwang added inline comments.
lld/ELF/CallGraphSort.cpp
244 ↗(On Diff #190644)

Because without this checking, I can get two symbols in the file that both match section. One of them is empty name. I wanted to use this filter out that symbol. Plus the goal of this part is to find out a symbol name from the file that is in the section. So empty-named symbols should be ignored?

tcwang updated this revision to Diff 192531.Mar 27 2019, 3:51 PM

Fix comments to filter out section-typed symbols, instead of empty named ones.

tcwang marked an inline comment as done.Mar 27 2019, 3:52 PM
tcwang added inline comments.
lld/ELF/CallGraphSort.cpp
244 ↗(On Diff #190644)

Change the empty name check to symbol type check. If a symbol is section, no need to do the matching below.

ruiu added inline comments.Mar 27 2019, 4:00 PM
lld/ELF/CallGraphSort.cpp
243 ↗(On Diff #192531)

So the cost of this code is O(n*m) where n is there number of the sorted sections and n is the number of the symbols in each file. I don't think this code needs to be super fast, as this is not in the usual code path. If the performance becomes a problem, let's revisit to optimize. I'm ok with this code for now.

lld/ELF/Driver.cpp
1475 ↗(On Diff #192531)

I don't think we need this guard, as this is a minor feature. Can you remove this code?

tcwang updated this revision to Diff 192533.Mar 27 2019, 4:05 PM
tcwang marked an inline comment as done.

Fix comments.

Bigcheese added inline comments.Mar 27 2019, 4:05 PM
lld/ELF/Driver.cpp
1475 ↗(On Diff #192531)

Why not be consistent here?

ruiu accepted this revision.Mar 27 2019, 4:07 PM

LGTM

This revision is now accepted and ready to land.Mar 27 2019, 4:07 PM
ruiu added inline comments.Mar 27 2019, 4:44 PM
lld/ELF/Driver.cpp
1475 ↗(On Diff #192531)

We could have many more features that writes something to a file, but I don't want to add two lines of code every time we add such feature. I'd remove MapFile rather than adding all files here if it needs to be consistent.

This revision was automatically updated to reflect the committed changes.