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

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
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.
----------------
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145379