[PATCH] D81429: [COFF] Port CallGraphSort to COFF from ELF

[Hans Wennborg via Phabricator via llvm-commits]

hans added a comment.

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



================
Comment at: lld/COFF/CallGraphSort.cpp:95
+// Maximum cluster size in bytes.
+constexpr uint64_t MAX_CLUSTER_SIZE = 1024 * 1024;
+} // end anonymous namespace
----------------
MaskRay wrote:
> This is pretty arbitrary as well.
I think it's fine to start by copying these values from ELF, and possibly tuning in the future.


================
Comment at: lld/COFF/CallGraphSort.cpp:198
+    // Don't consider merging if the edge is unlikely.
+    if (c.bestPred.from == -1 || c.bestPred.weight * 10 <= c.initialWeight)
+      continue;
----------------
zequanwu wrote:
> MaskRay wrote:
> > The coefficient is pretty arbitrary. Best if you can test which constant works well.
> What is the tool I can use for testing this?
Since this is a port of the ELF code, I think re-using the coefficient is fine, and tuning can be done later.


================
Comment at: lld/COFF/CallGraphSort.cpp:12
+/// https://research.fb.com/wp-content/uploads/2017/01/cgo2017-hfsort-final1.pdf
+///
+/// The goal of this algorithm is to improve runtime performance of the final
----------------
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.


================
Comment at: lld/COFF/CallGraphSort.cpp:73
+  int prev;
+  size_t size = 0;
+  uint64_t weight = 0;
----------------
Since the constructor initializes size, maybe the initializer here could be dropped?


================
Comment at: lld/COFF/CallGraphSort.cpp:118
+  for (std::pair<SectionPair, uint64_t> &c : profile) {
+    const auto *fromSB = cast<SectionChunk>(c.first.first->repl);
+    const auto *toSB = cast<SectionChunk>(c.first.second->repl);
----------------
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?


================
Comment at: lld/COFF/CallGraphSort.cpp:226
+  DenseMap<const SectionChunk *, int> orderMap;
+  int curOrder = INT_MIN;
+  for (int leader : sorted)
----------------
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.


================
Comment at: lld/COFF/CallGraphSort.cpp:227
+  int curOrder = INT_MIN;
+  for (int leader : sorted)
+    for (int i = leader;;) {
----------------
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.


================
Comment at: lld/COFF/CallGraphSort.cpp:253
+            if (d->isCOMDAT && !d->getCOFFSymbol().isSection())
+              if (sc == d->getChunk())
+                os << sym->getName() << "\n";
----------------
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.


================
Comment at: lld/COFF/Driver.cpp:945
+    }
+
+    if (DefinedCOFF *dr = dyn_cast_or_null<DefinedCOFF>(sym))
----------------
ELF calls maybeWarnUnorderableSymbol() here, but I suppose there's no corresponding concept for COFF?


================
Comment at: lld/test/COFF/cgprofile-icf.s:2
+# REQUIRES: x86
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-win32 %s -o %t
----------------
Could you add a comment here that explains the intention of the test?


================
Comment at: lld/test/COFF/cgprofile-txt.s:2
+# REQUIRES: x86
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-win32 %s -o %t
----------------
A short comment here which explains what the test is testing would be useful.


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

https://reviews.llvm.org/D81429