[PATCH] D117354: [lld-macho] Allow order files and call graph sorting to be used together

[Nico Weber via Phabricator via llvm-commits]

thakis accepted this revision.
thakis added a comment.

Code looks fine to me.

I tried it on Chromium, and apparently there are 542489 from all cg_profile section edges in the .o files, leading to 350079 entries in `config->callGraphProfile`. The edges are between 122620 isecs, compared to 918787 symbols total. So 800K symbols don't end up with an edge and just 122K do. That feels surprisingly low to me.

(The 542489 seems surprisingly low already: There's ~40k TUs, so that's just 10 call edges per TU on average.)

Maybe I'm holding something wrong locally.



================
Comment at: lld/MachO/SectionPriorities.cpp:290
+          (hasOrderFile &&
+           (getSymbolPriority(fromSym) || getSymbolPriority(toSym))))
         continue;
----------------
I would've expected this to be expensive: In Chromium Framework, we have ~500K of these edges, so this is 1M calls to getSymbolPriority() (which we later call once more for every symbol; there's ~920K symbols). But to my surprise, I can't measure a slowdown from it, so I suppose it's ok.

(The alternative would be to store the symbol's priority in the symbol or its isec and to compute it just once. But, no need, apparently.)


================
Comment at: lld/MachO/SectionPriorities.cpp:376
       return;
-
-    SymbolPriorityEntry &entry = it->second;
-    size_t &priority = sectionPriorities[sym.isec];
-    priority =
-        std::max(priority, getSymbolPriority(entry, sym.isec->getFile()));
+    size_t &priority = sectionPriorities[sym->isec];
+    priority = std::max(priority, symbolPriority.getValue());
----------------
When does this return non-0? The call graph order omits edges where either start or end have a priority from the order file, so it can only happen if two distinct symbols have the same isec, right? Does this run after ICF? Are there things other than ICF that could trigger it?


================
Comment at: lld/MachO/SectionPriorities.cpp:253
 
+static Optional<size_t> getSymbolPriority(Defined& sym) {
+    if (sym.isAbsolute())
----------------
oontvoo wrote:
> lgrey wrote:
> > oontvoo wrote:
> > > you're not modifying the symbol
> > Done (but for my edification: why pointer to const vs. const ref?)
> (commented below also - I suggested const* because the objects themselves are stored as pointers, so you can avoid the whole deref + taking address trip. Also it's not common to see const ref in lld code)
(fwiw, i think references for things that can't be null is fine)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117354/new/

https://reviews.llvm.org/D117354