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

[Sergey Pupyrev via Phabricator via llvm-commits]

spupyrev added inline comments.


================
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.
----------------
rahmanl wrote:
> spupyrev wrote:
> > rahmanl wrote:
> > > Please introduce cl::opts for these so they can be manually configured.
> > Based on my experience, exposing such constants doesn't add much value but may confuse some people. For example, C^3 has a bunch of similar options tuned once on a specific benchmark years ago, and no one has ever tried to re-consider them :) A have similar experience for other algorithms, where the defaults were never touched. At this time I consider the values as some internal algorithm-specific constants that are not supposed to be modified.
> > However, cl:opts take just a few lines of code, so i'm happy to add those if you have an alternative opinion.
> > 
> The configurations here look very architecture-dependent. So I am assuming that as hardware improves, we may increase these parameters to get a better result. WDYT?
Added


================
Comment at: llvm/lib/Transforms/Utils/CodeLayout.cpp:1294
+    // Merge the edges
+    Into->mergeEdges(From);
+    From->clear();
----------------
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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152834