[PATCH] D145379: [FuncSpec] Cost model improvements.

[Alexandros Lamprineas via Phabricator via llvm-commits]

labrinea added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/FunctionSpecialization.h:51
+// To compute the codesize increase, we first estimate the initial codesize of
+// the function as [number of instructions] + [number of inline candidates] x
+// [small function size]. The "small function size" corresponds to the minimum
----------------
SjoerdMeijer wrote:
> Thanks for adding these descriptions, that is very helpful.
> 
> Just trying to understand the initial code size calculation better, the last part:
> 
>    [number of inline candidates] x [small function size]
> 
> why is that not the [number of instructions] for each inline candidate?
Because this information is not available in CodeMetrics. We don't know who that inline candidate is at this point, and if we did we would have to compute it's CodeMetrics. So instead we use `mall function size` as an approximation. Knowing the exact number of instructions does not matter that much. What matters is to increase the codesize hit if there are inline candidates, meaning the entire function might end up even larger than it is now.


================
Comment at: llvm/include/llvm/Transforms/IPO/FunctionSpecialization.h:87
+// keeps the discovered specialisation opportunities for the module in a single
+// vector, where the specialisations of each function form a contiguous range.
+// This map's value is the beginning and the end of that range.
----------------
SjoerdMeijer wrote:
> It's not clear to me what this range exactly is.
This typdef is unrelated to this patch, I just grouped all the typedef here. Just ignore it.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:389
+  // specialisations.
+  DenseMap<SpecSig, unsigned> UM;
+
----------------
SjoerdMeijer wrote:
> Nit: `UM` is a bit cryptic. 
UniqueMap. Was already named as such before this patch, so unrelated. Please ignore on this review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145379