This is an archive of the discontinued LLVM Phabricator instance.

[lld][Macho][NFC] Encapsulate priorities map in a priority class
ClosedPublic

Authored by Roger on Mar 21 2022, 10:12 AM.

Details

Summary

config->priorities has been used to hold the intermediate state during the construction of the order in which sections should be laid out. This is not a good place to hold this state since the intermediate state is not a "configuration" for LLD. It should be encapsulated in a class for building a mapping from section to priority (which I created in this diff as the PriorityBuilder class).

The same thing is being done for config->callGraphProfile.

Diff Detail

Event Timeline

Roger created this revision.Mar 21 2022, 10:12 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 21 2022, 10:12 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Roger requested review of this revision.Mar 21 2022, 10:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 21 2022, 10:12 AM
Roger retitled this revision from Encapsulate priorities map to [lld][Macho] Encapsulate priorities map in a priority class.Mar 21 2022, 10:19 AM
Roger edited the summary of this revision. (Show Details)
Roger retitled this revision from [lld][Macho] Encapsulate priorities map in a priority class to [lld][Macho][NFC] Encapsulate priorities map in a priority class.Mar 21 2022, 10:36 AM
int3 accepted this revision.Mar 21 2022, 11:28 AM
int3 added a subscriber: int3.
int3 added inline comments.
lld/MachO/SectionPriorities.cpp
37–38

I don't think this needs to be a pointer (can just be a PriorityBuilder)

This revision is now accepted and ready to land.Mar 21 2022, 11:28 AM
Roger updated this revision to Diff 417053.Mar 21 2022, 12:40 PM

Added config->callGraphProfile to PriorityBuilder

Roger requested review of this revision.Mar 21 2022, 12:42 PM
Roger edited the summary of this revision. (Show Details)

I applied the same idea to config->callGraphProfile. @int3 Could you take a look?

Roger updated this revision to Diff 417059.Mar 21 2022, 12:56 PM

Change PriorityBuilder from a unique pointer to a regular object.

Roger updated this revision to Diff 417067.Mar 21 2022, 1:12 PM

Remove duplicated code representing "maximum priority"

Roger planned changes to this revision.Mar 21 2022, 1:35 PM
Roger requested review of this revision.Mar 21 2022, 3:32 PM
int3 added inline comments.Mar 21 2022, 6:57 PM
lld/MachO/SectionPriorities.cpp
304–305

I think you're confused about the intention of the code. numeric_limits::max() is the *highest* priority, not the lowest. But it's used to initialize the default value of the lowestPriority variable because if there are no entries in the order file, then the lowest priority is also the highest priority.

Now that we have the PriorityBuilder class, I think lowestPriority can be a member there. And maybe add a comment so future readers won't get confused :)

Roger marked an inline comment as done.Mar 22 2022, 8:33 AM
Roger added inline comments.
lld/MachO/SectionPriorities.cpp
304–305

lowestPriority actually represents the highest priority that has not been assigned (ie, the priority lower than the lowest priority that has so far been assigned). lowestPriority is used to keep track of the highest priority CallGraphSort should use because the code aims to make sure that all priorities assigned by CallGraphSort should be lower than those assigned by the order file.

At the time this line of code is called, lowestPriority will always be set to its initial value of std::numeric_limits<size_t>::max(). There is no need to specify std::numeric_limits<size_t>::max() both here and at the initialization of lowestPriority.

I considered making lowestPriority a member of PriorityBuilder but it would be a bit more than just moving the field and changing some names because lowestPriority is read and written to by CallGraphSort as well. I didn't include that change here but it could possibly be done.

thevinster added inline comments.
lld/MachO/SectionPriorities.cpp
37

nit: Since we are already namespacing macho, can we drop the macho:: everywhere in this file?

Roger marked an inline comment as done.Mar 22 2022, 9:02 AM
Roger added inline comments.
lld/MachO/SectionPriorities.cpp
37

the code does not compile without macho::. I'm not sure exactly why but it appears to be necessary.

int3 accepted this revision.Mar 22 2022, 9:03 AM
int3 added inline comments.
lld/MachO/SectionPriorities.cpp
37

not for declarations :)

macho:: here tells the compiler that this is a declaration in the macho:: namespace, not the global one.

304–305

ah okay, I see how you're thinking about it. I still think priority = lowestPriority is confusing though, because it's not obvious that we really mean "lowest priority unassigned by the order file" instead of "lowest possible priority". Also come to think of it, the local priority variable is kind of redundant. How about using lowestPriority directly but renaming it to something like lowestUnusedPriority?

This revision is now accepted and ready to land.Mar 22 2022, 9:03 AM
Roger marked an inline comment as done.Mar 22 2022, 9:03 AM
Roger added inline comments.
lld/MachO/SectionPriorities.cpp
304–305

Actually, it may be better to rename lowestPriority to highestAvailablePriority.

int3 added inline comments.Mar 22 2022, 9:05 AM
lld/MachO/SectionPriorities.cpp
304–305

sgtm

Roger updated this revision to Diff 417317.Mar 22 2022, 9:13 AM
Roger marked 2 inline comments as done.

Rename lowestPriority to highestAvailablePriority.

Roger added inline comments.Mar 22 2022, 9:15 AM
lld/MachO/SectionPriorities.cpp
304–305

As I was reading the code, I got the sense that assigning priority = lowestPriority was done intentionally. I was wondering if this is a speed optimization (incrementing the stack copy priority many times and writing to the global lowestPriority just at the end). WDYT?

int3 added inline comments.Mar 22 2022, 9:26 AM
lld/MachO/SectionPriorities.cpp
304–305

As I was reading the code, I got the sense that assigning priority = lowestPriority was done intentionally. I was wondering if this is a speed optimization (incrementing the stack copy priority many times and writing to the global lowestPriority just at the end). WDYT?

I don't think you posted this comment before I replied (my "sgtm" was in response to the rename), but it seems like Phab has recorded them out of order...

I doubt it's an intentional speed optimization; the difference is really not that significant, and I would expect a comment or something if profiling had indeed identified this as a bottleneck. Otherwise it's just unnecessary microoptimization :)

Also given that it's a static, there's a decent chance the compiler can optimize it anyway.

Roger updated this revision to Diff 417352.Mar 22 2022, 11:38 AM

Get rid of unnecessary priority variable.

Roger marked an inline comment as done.Mar 22 2022, 11:41 AM
Roger added inline comments.
lld/MachO/SectionPriorities.cpp
304–305

Gotcha. I removed the priority variable.

Roger marked an inline comment as done.Mar 22 2022, 11:41 AM
int3 added inline comments.Mar 22 2022, 1:41 PM
lld/MachO/SectionPriorities.cpp
252

oh yeah -- unlike global functions, class method definitions don't actually need the macho:: prefix (I believe because clang first performs name lookup on PriorityBuilder, so it knows in which namespace this code belongs to)