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

[Rahman Lavaee via Phabricator via llvm-commits]

rahmanl accepted this revision.
rahmanl added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/lib/Transforms/Utils/CodeLayout.cpp:1346
+  /// Config for the algorithm.
+  const CDSortConfig &Config;
+
----------------
`const CDSortConfig Config` to avoid dangling references.


================
Comment at: llvm/lib/Transforms/Utils/CodeLayout.cpp:1464
+  // Populate the config from the command-line options.
+  if (CacheEntries.getNumOccurrences() > 0)
+    Config.CacheEntries = CacheEntries;
----------------
We don't need these I think.


================
Comment at: llvm/lib/Transforms/Utils/CodeLayout.cpp:1294
+    // Merge the edges
+    Into->mergeEdges(From);
+    From->clear();
----------------
spupyrev wrote:
> rahmanl wrote:
> > 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`.
> As far as I can see, `ChainT::mergeEdges` does a bit more than removing the items from the vectors; I don't see a simple way of reducing the complexity here. (But I am open for reviewing speedups of course)
Sounds good. I'll take a stab at it later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152834