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

[Rahman Lavaee via Phabricator via llvm-commits]

rahmanl added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/CodeLayout.cpp:1010
+    // Collect nodes from all the chains
+    concatChains(Result);
+  }
----------------
Although this is pre-existing, I wonder why this is returning by parameter.


================
Comment at: llvm/lib/Transforms/Utils/CodeLayout.cpp:1065-1067
+      if (Node.ExecutionCount > 0) {
+        HotChains.push_back(&AllChains.back());
+      }
----------------
Drop braces for single-statement ifs.


================
Comment at: llvm/lib/Transforms/Utils/CodeLayout.cpp:1101
+    for (ChainT *ChainPred : HotChains) {
+      for (auto EdgeIt : ChainPred->Edges) {
+        ChainEdge *Edge = EdgeIt.second;
----------------
Same here.


================
Comment at: llvm/lib/Transforms/Utils/CodeLayout.cpp:1135
+      // Remove outdated edges from the queue
+      for (std::pair<ChainT *, ChainEdge *> EdgeIt : BestSrcChain->Edges)
+        Queue.erase(EdgeIt.second);
----------------
How about we simply this using:
`for (const auto &[Chain, ChainEdge]: BestSrcChain->Edges) Queue.erase(ChainEdge);`


================
Comment at: llvm/lib/Transforms/Utils/CodeLayout.cpp:1146
+      // Insert newly created edges into the queue
+      for (auto EdgeIt : BestSrcChain->Edges) {
+        ChainEdge *Edge = EdgeIt.second;
----------------
`const auto & [Chain, Edge]` to avoid unnecessary copies and improve readability.


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


================
Comment at: llvm/lib/Transforms/Utils/CodeLayout.cpp:1294
+    // Merge the edges
+    Into->mergeEdges(From);
+    From->clear();
----------------
I wonder if this could be a source of performance drop for larger programs. These chains are much bigger than the block chains and could have more edges with other chains. However, we currently use std::vector for `ChainT::Edges` and removing things from a vector one by one could result in quadratic time complexity. So a potential optimization is to remove all outdated edges in one shot using the combination of `vector::erase` and `std::remove_if`.


================
Comment at: llvm/lib/Transforms/Utils/CodeLayout.cpp:1322
+    // Sort chains by density in the decreasing order
+    std::stable_sort(SortedChains.begin(), SortedChains.end(),
+                     [&](const ChainT *L, const ChainT *R) {
----------------
We don't need to use stable_sort if we have tie-breaking by identifiers.


================
Comment at: llvm/lib/Transforms/Utils/CodeLayout.cpp:1333
+    Order.reserve(NumNodes);
+    for (const ChainT *Chain : SortedChains) {
+      for (NodeT *Node : Chain->Nodes) {
----------------
Just use braces for the outer loop per LLVM standards: https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements


================
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();
----------------
spupyrev wrote:
> rahmanl wrote:
> > nit: missing period.
> I'm using commas only for method-level comments (`///`). Is there a standard/preference around this?
I'd use the correct punctuation everywhere per guidance from https://llvm.org/docs/CodingStandards.html#commenting
A comment ending without period may imply it's been truncated. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152834