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

[Sergey Pupyrev via Phabricator via llvm-commits]

spupyrev marked an inline comment as done.
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:
> 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.



================
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();
----------------
rahmanl wrote:
> nit: missing period.
I'm using commas only for method-level comments (`///`). Is there a standard/preference around this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152834