[PATCH] D139346: [FuncSpec] Global ranking of specialisations

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

Agreed, nice change, some nits inlined.



================
Comment at: llvm/include/llvm/Transforms/IPO/FunctionSpecialization.h:84
+// Specialization instance.
+struct Spec {
+  // Original function.
----------------
Nit: perhaps a more descriptive name. The original SpecInfo was a tiny bit better.


================
Comment at: llvm/include/llvm/Transforms/IPO/FunctionSpecialization.h:106
 
-using CallSpecBinding = std::pair<CallBase *, SpecializationInfo>;
-// We are using MapVector because it guarantees deterministic iteration
-// order across executions.
-using SpecializationMap = SmallMapVector<CallBase *, SpecializationInfo, 8>;
+// Map of potential specializations for each function. Value is the range
+// [start, end) in a module-wide specialisations array.
----------------
Nit: range of what exactly? I don't know what a "specialisations array" is.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:303-304
+  // TODO: Currently "budget" is derived from max number of specializations per
+  // function. A more reasonable metric would be acceptable increase in code
+  // size.
+  auto CompareGain = [&AllSpecs](unsigned I, unsigned J) {
----------------
The inline cost used to be a fundamental part of the cost-model. In getSpecializationBonus, you will see calls to getInlineCost, which I hope takes increase in code size into account. 


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

https://reviews.llvm.org/D139346