[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:
> labrinea wrote:
> > 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.
> Thanks, if you can add this explanation to the comments that would help. 
Will do.


================
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:
> chill wrote:
> > SjoerdMeijer wrote:
> > > labrinea wrote:
> > > > 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.
> > > Okay, so it lived somewhere else before and you just moved it here, but can you explain what the range is while we are at it?
> > Isn't that clear from the comment above?
> > 
> > We have a vector of specializations. In this vector the specializations of any one function are adjacent to one another, IOW "form a contiguous range".
> > For example, if specializations of  `foo` are at indices 4, 5, 6, 7 in the vector, we keep just 4 and 8,
> > to save space. The range is `[4, 8)`.
> > 
> > A somewhat longer description of the data structures is given in the commit message: in https://reviews.llvm.org/rGe6b9fc4c8be00660c5e1f1605a6a5d92475bdba7
> > 
> > Isn't that clear from the comment above?
> 
> No, sorry, it wasn't for me. This however, is crystal clear to me:
> 
> > We have a vector of specializations. In this vector the specializations of any one function are adjacent to one another, IOW "form a contiguous range". For example, if specializations of  foo are at indices 4, 5, 6, 7 in the vector, we keep just 4 and 8, to save space. The range is [4, 8).
> 
> As it also answers my follow up question, i.e. what is the relevance/rationale of this. I think exactly this should be part of the comment/explanation here.
> 
> > A somewhat longer description of the data structures is given in the commit message: in https://reviews.llvm.org/rGe6b9fc4c8be00660c5e1f1605a6a5d92475bdba7
> 
> So I missed that review, i.e. didn't have time to look into it, but am now dipping back into FuncSpec, which is why I am missing some rationale. But I think this is an excellent write-up, that should be, or parts of it, should be part of the write up to make this self contained (i.e. it's a bit of a missed opportunity this explanation is only in the commit message).
I disagree with repeating the write-up in a comment as it may become irrelevant at some point. I think git blame should suffice. I find the existing comment already quite extensive/informative.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:389
+  // specialisations.
+  DenseMap<SpecSig, unsigned> UM;
+
----------------
SjoerdMeijer wrote:
> labrinea wrote:
> > SjoerdMeijer wrote:
> > > Nit: `UM` is a bit cryptic. 
> > UniqueMap. Was already named as such before this patch, so unrelated. Please ignore on this review.
> Naming is difficult, but it would be a shame if we cannot come up with something better while we are at it.
Agreed. I'll update it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145379