This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Add --{,no-}call-graph-profile-sort (enabled by default)
ClosedPublic

Authored by MaskRay on Oct 24 2018, 5:06 PM.

Event Timeline

MaskRay created this revision.Oct 24 2018, 5:06 PM
MaskRay updated this revision to Diff 171028.Oct 24 2018, 5:14 PM

Two tests

I really like adding the option to disable it, but I'm not sure about disabling it by default. Is there any situation where the profile is present where a user wouldn't want to perform layout? The algorithm is very fast so unexpected link time increase isn't really an issue, and the exact order of sections already isn't guaranteed by ELF and differs between linkers so I don't expect there to be correctness issues.

grimar added a subscriber: grimar.Oct 25 2018, 4:09 AM
ruiu added a comment.Oct 25 2018, 11:00 AM

Speaking of whether we should enable or disable it by default, I second Michael. In most cases, if your object files contain .llvm.call-graph-profile sections, that's because you are instructed clang/llvm to do so, and if lld doesn't reorder sections in that case, most users wonder why lld doesn't work "correctly". From that UX perspective, that should be enabled by default.

MaskRay added a subscriber: pcc.Oct 25 2018, 11:03 AM
MaskRay updated this revision to Diff 171146.Oct 25 2018, 11:10 AM

enable by default

MaskRay updated this revision to Diff 171147.Oct 25 2018, 11:10 AM
MaskRay retitled this revision from [ELF] Add --{,no-}call-graph-profile (disabled by default) to [ELF] Add --{,no-}call-graph-profile (enabled by default).
MaskRay edited the summary of this revision. (Show Details)
MaskRay removed subscribers: pcc, grimar.

Update title

ruiu added inline comments.Oct 25 2018, 11:14 AM
ELF/Driver.cpp
789

nit: you probably should spend a bit more time to read existing code around the location where you add new code to make your new code consistent with existing one. All the other Config members have the exact same name with a corresponding command line flag. For example, Config->EhFrameHdr corresponds to --eh-frame-hdr and Config->EmitRelocs corresponds to --emit-relocs. Please make your new code consistent with existing code.

MaskRay added inline comments.Oct 25 2018, 11:16 AM
ELF/Driver.cpp
789

I know, but the variable CallGraphProfile is already used as a map, what should I rename it to?

MaskRay updated this revision to Diff 171150.Oct 25 2018, 11:21 AM

Name the bool Config->CallGraphProfile and rename existing COnfig->CallGraphProfile to CallGraphInfo

ELF/Driver.cpp
789
bool CallGraphProfile;
llvm::MapVector<std::pair<const InputSectionBase *, const InputSectionBase *>,
                  uint64_t>
    CallGraphInfo;

Let me know if you have other preference

ruiu added inline comments.Oct 25 2018, 11:28 AM
ELF/Driver.cpp
789

Yeah you should, because you essentially should keep code beautiful/readable for those who read this code for the first time. If in doubt, forget about history of code and see new code with fresh eye. I'm sure you'll find this code inconsistent and odd.

Bigcheese added inline comments.Oct 25 2018, 12:54 PM
ELF/Driver.cpp
789

This is a much less descriptive name. I would call the option {-no}-call-graph-profile-sort as you are disabling the sorting based on the profile, not the profile itself. Then the option name becomes CallGraphProfileSort.

ruiu added a comment.Oct 25 2018, 1:02 PM

I like that flag name.

MaskRay updated this revision to Diff 171177.Oct 25 2018, 1:32 PM
MaskRay retitled this revision from [ELF] Add --{,no-}call-graph-profile (enabled by default) to [ELF] Add --{,no-}call-graph-profile-sort (enabled by default).
MaskRay edited the summary of this revision. (Show Details)

Use option name suggested by Bigcheese

MaskRay marked 5 inline comments as done.Oct 25 2018, 1:32 PM
MaskRay updated this revision to Diff 171178.Oct 25 2018, 1:33 PM

Revert a comment change

ruiu added inline comments.Oct 25 2018, 1:34 PM
ELF/CallGraphSort.cpp
99

Unrelated change?

MaskRay marked an inline comment as done.Oct 25 2018, 1:44 PM
MaskRay added inline comments.
ELF/CallGraphSort.cpp
99

Sorry I noticed it in my "Revert a comment change" but maybe that command failed

ruiu added a comment.Oct 25 2018, 1:48 PM

Looks good. Michael?

MaskRay updated this revision to Diff 171194.Oct 25 2018, 2:08 PM
MaskRay marked an inline comment as done.

Change the text file from cgprofile-obj-warn.s to cgprofile-obj.s as I will delete cgprofile-obj-warn.s in https://reviews.llvm.org/D53669

Use llvm-nm instead of llvm-readobj to check symbol addresses

MaskRay updated this revision to Diff 171205.Oct 25 2018, 3:21 PM

Rebase as it would otherwise have to touch file touched by D53669

MaskRay updated this revision to Diff 171211.Oct 25 2018, 3:53 PM

Use llvm-nm --no-sort

This revision is now accepted and ready to land.Oct 25 2018, 4:13 PM
This revision was automatically updated to reflect the committed changes.