This is an archive of the discontinued LLVM Phabricator instance.

[COFF] Port CallGraphSort to COFF from ELF
ClosedPublic

Authored by zequanwu on Jun 8 2020, 1:51 PM.

Details

Diff Detail

Event Timeline

zequanwu created this revision.Jun 8 2020, 1:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2020, 1:51 PM
MaskRay added inline comments.Jun 8 2020, 3:46 PM
lld/COFF/CallGraphSort.cpp
95

This is pretty arbitrary as well.

198

The coefficient is pretty arbitrary. Best if you can test which constant works well.

zequanwu added inline comments.Jun 9 2020, 9:34 AM
lld/COFF/CallGraphSort.cpp
198

What is the tool I can use for testing this?

zequanwu updated this revision to Diff 269601.Jun 9 2020, 10:48 AM

Templatize CallGraphSort and put it in Common directory.

zequanwu edited the summary of this revision. (Show Details)Jun 9 2020, 10:50 AM

I am on the fence about reusing the code. Adding some people for feedback.

MaskRay added inline comments.Jun 9 2020, 12:45 PM
lld/ELF/CallGraphSort.cpp
226

Why is it INT_MIN now?

lld/include/lld/Common/CallGraphSort.h
15 ↗(On Diff #269601)

Avoid using namespace in a header file

zequanwu updated this revision to Diff 269659.Jun 9 2020, 1:53 PM
zequanwu marked an inline comment as done.
zequanwu edited the summary of this revision. (Show Details)

Move using namespace llvm; from header file to cpp file.

zequanwu marked an inline comment as done and an inline comment as not done.Jun 9 2020, 1:54 PM
zequanwu added inline comments.
lld/ELF/CallGraphSort.cpp
226

The order of section is sorted in increasing order, so this does not change the behavior.

Because I reuse the sortBySectionOrder of COFF's /orderoption to do sorting (https://github.com/llvm/llvm-project/blob/master/lld/COFF/Writer.cpp#L656). It assigns symbols in the given file priorities starting from INT_MIN and non present symbols get priority 0. It also sorts section in increasing order.
Or I can leave int curOrder = 1, but when copying the numbers, subtracting each number by INT_MIN.

A a few comments/suggestions.

lld/ELF/CallGraphSort.cpp
104

Why T and not something like Section, similar to Symbol?
Perhaps I'd use TSection and TSymbol to make clear they are template names.

lld/ELF/Writer.cpp
1310 ↗(On Diff #269659)

I think it is common for LLD ELF to ise sec for sections. So sc -> sec probably.
I'd also leave a comment to explain for what this predicate is used. Perhaps it is better not to inline it,
but define separatelly. Like

 // A comment.
auto PrintSymbolOrderPred = [] (...) { ... };
...

return computeCallGraphProfileOrder<InputSectionBase, Symbol>(
        config->callGraphProfile, config->printSymbolOrder, PrintSymbolOrderPred );
lld/include/lld/Common/CallGraphSort.h
63 ↗(On Diff #269659)

There is no /callgraph-profile-file option in ELF (it has --call-graph-ordering-file).

Since this is a common code, comments should be applicable for all platforms.

72 ↗(On Diff #269659)

I think it needs a comment about what is printSymbolOrder and what is cond for.

cond here is used as a filter predicate for --print-symbol-order. Its name is probably not clear.

grimar added a comment.EditedJun 10 2020, 4:18 AM

I am on the fence about reusing the code. Adding some people for feedback.

I am also not sure it is a good thing to do. It seems makes LLD ELF code a bit less readable.
I do not really like how --print-symbol-order is handled with this.

zequanwu updated this revision to Diff 269943.Jun 10 2020, 12:21 PM

Address comments.

zequanwu marked 3 inline comments as done.Jun 10 2020, 12:23 PM
zequanwu marked an inline comment as done.Jun 10 2020, 12:31 PM

Parametric generalization on InputSection and Symbol may not be a good idea but they are very different in ELF and COFF. ICF.cpp is an example where we could reuse code but we choose not to.
If it is not clear reusing code will be beneficial, I think we can duplicate code for now. The algorithm is simple and there is not much duplication anyway.

zequanwu updated this revision to Diff 269969.Jun 10 2020, 2:24 PM

I agree to adopt how ICF.cpp does.
roll back to previous version to have CallGraphSort in both ELF and COFF.

zequanwu edited the summary of this revision. (Show Details)Jun 15 2020, 5:17 PM
zequanwu added a reviewer: hans.
hans added a comment.Jun 16 2020, 7:07 AM

This is looking good to me overall. Just some minor comments.

lld/COFF/CallGraphSort.cpp
12

Even if we're duplicating the code, I would suggest not duplicating the full comment, but put something like "this is based on the ELF port, see lld/ELF/CallGraphSort.cpp" or something like that.

73

Since the constructor initializes size, maybe the initializer here could be dropped?

95

I think it's fine to start by copying these values from ELF, and possibly tuning in the future.

118

The "SB" here and otherwhere is a bit confusing since I guess it stands for SectionBase in the original code, but here we're using SectionChunk. Maybe instead of "SB", "Sec" would be more clear?

198

Since this is a port of the ELF code, I think re-using the coefficient is fine, and tuning can be done later.

226

Since starting from INT_MIN here is different than in ELF, I think it would be good to at least have a comment explaining why.

227

I see the ELF code also has no curly braces around the for loop, but I think it would help readability, so I'd suggest adding them both here and in the original code.

253

Instead of nesting the if's maybe use "if (a && b)". Also maybe a comment about what you're filtering for here would be helpful.

lld/COFF/Driver.cpp
945

ELF calls maybeWarnUnorderableSymbol() here, but I suppose there's no corresponding concept for COFF?

lld/test/COFF/cgprofile-icf.s
3

Could you add a comment here that explains the intention of the test?

lld/test/COFF/cgprofile-txt.s
3

A short comment here which explains what the test is testing would be useful.

zequanwu updated this revision to Diff 271152.Jun 16 2020, 11:38 AM
zequanwu marked 10 inline comments as done.

Address comments and rebase.

lld/COFF/Driver.cpp
945

I don't find corresponding concept.

lld/test/COFF/cgprofile-icf.s
3

This is ported from ELF/cgprofile-icf.s. I suppose the intention is to test the compatibility of ICF and cgprofile

zequanwu updated this revision to Diff 271154.Jun 16 2020, 11:40 AM

remove a dummy file.

MaskRay added inline comments.Jun 16 2020, 6:07 PM
lld/COFF/Driver.cpp
945

maybeWarnUnorderableSymbol is not useful. You can omit it for COFF. The list is usually generated by the compilers - in very few circumstances a user specifies it. There is not much point diagnosing unneeded symbols in the linker.

lld/ELF/CallGraphSort.cpp
71

uint64_t may be better.

size_t is 32-bit on a 32-bit platform, while you want a 64-bit value here.

zequanwu updated this revision to Diff 271265.Jun 16 2020, 7:34 PM

Change type of size in cluster from size_t to uint64_t.

Friendly ping.

MaskRay added inline comments.Jul 10 2020, 3:31 PM
lld/test/COFF/cgprofile-txt.s
22

Add -NEXT: whenever applicable

45

Delete trailing empty lines.

Please fix other files as well.

zequanwu updated this revision to Diff 277180.Jul 10 2020, 5:16 PM
zequanwu marked 2 inline comments as done.

Remove trailing empty lines.

lld/test/COFF/cgprofile-txt.s
22

The complete output is like the following, so there is a gap.

140001000 t .text
140001000 T B
140001001 t .text
140001001 T C
140001002 t .text
140001002 T D
140001003 t .text
140001003 T A
140001004 t .text
zequanwu updated this revision to Diff 279626.Jul 21 2020, 1:50 PM

Add option /call-graph-profile-sort in lld-link to read CGProfile data from object file (default on), equivalent to --call-graph-profile-sort in ld.lld.

hans accepted this revision.Jul 22 2020, 6:06 AM

I added some more very minor comments, but overall this looks good to me. Please wait for maskray to sign off too, though.

There doesn't seem to be good documentation for the ELF feature that I could find. I'm not sure where the best way to add it would be (lld's man page?) but if you want, that would be a good thing to improve.

lld/COFF/CMakeLists.txt
8

nit: move up one line to make it more alphabetic

lld/COFF/CallGraphSort.cpp
9–10

isn't it lld/ELF/CallGraphSort.cpp?

20

is this actually needed?

lld/COFF/Driver.cpp
932

nit: period at the end

2097

What happens if the user passes /order but there is also call graphs in the object files?

2118

nit: period.

2127

nit: period

lld/COFF/Options.td
247

The order could also come from the object files, right? I mean it doesn't have to be specified y /call-graph-ordering-file (but I guess the output is useful for passing to the flag)

This revision is now accepted and ready to land.Jul 22 2020, 6:06 AM
zequanwu updated this revision to Diff 280274.Jul 23 2020, 3:41 PM
zequanwu marked 9 inline comments as done.

Address comments.

zequanwu added inline comments.Jul 23 2020, 3:48 PM
lld/COFF/Options.td
247

Yes.

Friendly ping.

MaskRay accepted this revision.Jul 29 2020, 10:51 PM
MaskRay added inline comments.
lld/test/COFF/cgprofile-icf.s
3

Nit: Comments usually end with a full stop.

zequanwu updated this revision to Diff 282065.Jul 30 2020, 3:24 PM

Rebase and comment.

This revision was landed with ongoing or failed builds.Jul 30 2020, 3:24 PM
This revision was automatically updated to reflect the committed changes.