[PATCH] D36351: [lld][ELF] Add profile guided section layout

[Michael Spencer via Phabricator via llvm-commits]

Bigcheese added inline comments.


================
Comment at: ELF/CallGraphSort.cpp:145-151
+  std::sort(Nodes.begin(), Nodes.end(), [](const Node &A, const Node &B) {
+    return (APFloat(APFloat::IEEEdouble(), A.Weight) /
+            APFloat(APFloat::IEEEdouble(), A.Size))
+               .compare(APFloat(APFloat::IEEEdouble(), B.Weight) /
+                        APFloat(APFloat::IEEEdouble(), B.Size)) ==
+           APFloat::cmpLessThan;
+  });
----------------
ruiu wrote:
> Bigcheese wrote:
> > ruiu wrote:
> > > Do you actually need arbitrary precision floating point numbers?
> > We need deterministic results. This is the easiest way to get that in llvm.
> Are you saying that this function sorts `Nodes` in a different order if you use `double` instead? It is hard to imagine for me that  some two not-so-huge integers A and B can result in A<B in arbitrary precision calculation but in A==B || A > B if the same computation is done in `double`.
APFloat in this context isn't arbitrary precision. It's just a soft-float implementation of IEEE double which will behave bit identical on all platforms.

Looking at the rest of llvm I only see floating point math used in debug aids, TargetSchedule, and in ConstantFolding where there's a FIXME comment that says stop using it.

I'm fine using double if we know the final order will be the same on all platforms.


================
Comment at: ELF/Config.h:113
+  llvm::DenseMap<std::pair<llvm::StringRef, llvm::StringRef>, uint64_t>
+      CGProfile;
   bool AllowMultipleDefinition;
----------------
ruiu wrote:
> Bigcheese wrote:
> > ruiu wrote:
> > > Should be `CallgraphOrderingFile` as all Config members are named after their corresponding command line options.
> > That won't be the only way this member is updated, and it's not a file.
> Then can you just drop `File` and name this `CallgraphOrdering`?
It's also not an ordering. Which means the name of the option is wrong. Should be -callgraph-profile-file and `CallGraphProfile`.


Repository:
  rL LLVM

https://reviews.llvm.org/D36351