[PATCH] D152834: A new code layout algorithm for function reordering [2/3]

[Rahman Lavaee via Phabricator via llvm-commits]

rahmanl added a comment.

Thanks for the updated results. They look more consistent now.



================
Comment at: llvm/include/llvm/Transforms/Utils/CodeLayout.h:56
 
+/// Algorithm-specific params for Cache-Directed Sort. The values are tuned for
+/// the best performance of large-scale front-end bound binaries.
----------------
Please introduce cl::opts for these so they can be manually configured.


================
Comment at: llvm/lib/Transforms/Utils/CodeLayout.cpp:1097
+    auto GainComparator = [](ChainEdge *L, ChainEdge *R) {
+      if (L->gain() != R->gain())
+        return L->gain() > R->gain();
----------------
Here, you can use `std::make_tuple(-L->gain(), L->srcChain()->Id, L->destChain()->Id) < std::make_tuple(-R->gain(), R->srcChain()->Id, R->destChain()->Id)`.


================
Comment at: llvm/lib/Transforms/Utils/CodeLayout.cpp:1105
+    };
+    std::set<ChainEdge *, decltype(GainComparator)> Queue(GainComparator);
+
----------------
I wonder if we should make this a class member and then decompose this large function into smaller pieces.


================
Comment at: llvm/lib/Transforms/Utils/CodeLayout.cpp:1107
+
+    // Inserting the edges into the queue
+    for (ChainT *ChainPred : HotChains) {
----------------
nit: Use consistent style ("Insert" instead of "Inserting").


================
Comment at: llvm/lib/Transforms/Utils/CodeLayout.cpp:1183
+
+    // The object holds the best currently chosen gain of merging the two chains
+    MergeGainT Gain = MergeGainT();
----------------
nit: missing period.


================
Comment at: llvm/lib/Transforms/Utils/CodeLayout.cpp:1187
+    /// Given a merge offset and a list of merge types, try to merge two chains
+    /// and update Gain with a better alternative
+    auto tryChainMerging = [&](size_t Offset,
----------------
nit: missing period.


================
Comment at: llvm/lib/Transforms/Utils/CodeLayout.cpp:1188
+    /// and update Gain with a better alternative
+    auto tryChainMerging = [&](size_t Offset,
+                               const std::vector<MergeTypeT> &MergeTypes) {
----------------
Is this ever used with `Offset != 0`?


================
Comment at: llvm/lib/Transforms/Utils/CodeLayout.cpp:1238
+  /// Compute the change of the frequency locality after merging the chains.
+  double mergeGainFreq(ChainT *ChainPred, ChainT *ChainSucc) const {
+    auto missProbability = [&](double ChainDensity) {
----------------
Can you use a better name for this function? I am thinking of something like `computeFreqBasedLocalityGainForMerge` or `freqBasedLocalityGainForMerge`. It's longer but it fully captures the semantics.

Same comment about `mergeGainDist`.


================
Comment at: llvm/lib/Transforms/Utils/CodeLayout.cpp:1262
+  /// Compute the distance locality for a jump / call.
+  double distScore(uint64_t SrcAddr, uint64_t DstAddr, uint64_t Count) const {
+    uint64_t Dist = SrcAddr <= DstAddr ? DstAddr - SrcAddr : SrcAddr - DstAddr;
----------------
Use a more representative naming for this function.


================
Comment at: llvm/lib/Transforms/Utils/CodeLayout.cpp:1332-1335
+                       const double DL = ChainDensity[L];
+                       const double DR = ChainDensity[R];
+                       // Compare by density and break ties by chain identifiers
+                       return (DL != DR) ? (DL > DR) : (L->Id < R->Id);
----------------
Use `std::make_tuple` to compare these.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152834