[PATCH] D36351: [lld][ELF] Add profile guided section layout
Rui Ueyama via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Oct 3 18:43:33 PDT 2017
ruiu added a comment.
Could you send me a patch to produce a call graph file so that I can try this patch on my machine?
================
Comment at: ELF/CallGraphSort.cpp:99-101
+ if (To != Other.To)
+ return To < Other.To;
+ return false;
----------------
You can just return `To < Other.To`.
================
Comment at: ELF/CallGraphSort.cpp:127
+ // Create the graph.
+ for (const auto &C : Profile) {
+ if (C.second == 0)
----------------
This loop is a bit too dense. It cannot be understood without reading each line carefully as I don't understand the whole picture. Please insert a blank line between code blocks. Adding more comment would help.
================
Comment at: ELF/CallGraphSort.cpp:128
+ for (const auto &C : Profile) {
+ if (C.second == 0)
+ continue;
----------------
Please define local variables for `C.first.first`, `C.first.second` and `C.second` so that they are accessed through meaningful names.
================
Comment at: ELF/CallGraphSort.cpp:130-135
+ auto FromDR = dyn_cast_or_null<DefinedRegular>(Symtab->find(C.first.first));
+ auto ToDR = dyn_cast_or_null<DefinedRegular>(Symtab->find(C.first.second));
+ if (!FromDR || !ToDR)
+ continue;
+ auto FromSB = dyn_cast_or_null<const InputSectionBase>(FromDR->Section);
+ auto ToSB = dyn_cast_or_null<const InputSectionBase>(ToDR->Section);
----------------
auto -> auto *
================
Comment at: ELF/CallGraphSort.cpp:147
+ Nodes[To].IncidentEdges.push_back(EI);
+ } else
+ Edges[EI].Weight = SaturatingAdd(Edges[EI].Weight, C.second);
----------------
nit: add {}
================
Comment at: ELF/CallGraphSort.cpp:153
+
+void CallGraphSort::contractEdge(EdgeIndex CEI) {
+ // Make a copy of the edge as the original will be marked killed while being
----------------
Please add a function comment as to what this function is intended to do. I do not understand this function because I don't get a whole picture.
================
Comment at: ELF/CallGraphSort.cpp:158-163
+ // Remove the self edge from From.
+ FE.erase(std::remove(FE.begin(), FE.end(), CEI));
+ std::vector<EdgeIndex> &TE = Nodes[CE.To].IncidentEdges;
+ // Update all edges incident with To to reference From instead. Then if they
+ // aren't self edges add them to From.
+ for (EdgeIndex EI : TE) {
----------------
Add blank lines before comments.
================
Comment at: ELF/CallGraphSort.cpp:165-166
+ Edge &E = Edges[EI];
+ // E.From = E.From == CE.To ? CE.From : E.From;
+ // E.To = E.To == CE.To ? CE.From : E.To;
+ if (E.From == CE.To)
----------------
Please remove debug code.
================
Comment at: ELF/CallGraphSort.cpp:178-180
+ // Free memory.
+ std::vector<EdgeIndex>().swap(TE);
+
----------------
This looks odd. Why do you need to do this? I think you can just leave it alone.
================
Comment at: ELF/CallGraphSort.cpp:207-208
+
+// Group InputSections into clusters using the Call-Chain Clustering heuristic
+// then sort the clusters by density.
+void CallGraphSort::generateClusters() {
----------------
This might be understood for those who read the paper, but I don't think that is enough. Please write more comment as to what are clusters, what is density, and what is the heuristic.
================
Comment at: ELF/CallGraphSort.cpp:272-273
+DenseMap<const InputSectionBase *, int> elf::computeCallGraphProfileOrder() {
+ CallGraphSort CGS(Config->CallGraphProfile);
+ return CGS.run();
+}
----------------
You can do this in one line without defining a local variable.
================
Comment at: ELF/Writer.cpp:926-928
+ for (BaseCommand *Base : Script->Opt.Commands)
+ if (auto *OS = dyn_cast<OutputSection>(Base))
+ if (OS->Name == ".text") {
----------------
I'd factor this code out as `OutputSection *findOutputSection(StringRef Name)`.
https://reviews.llvm.org/D36351
More information about the llvm-commits
mailing list