[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: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
----------------
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. 


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


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:389
+  // specialisations.
+  DenseMap<SpecSig, unsigned> UM;
+
----------------
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.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:537
   auto *I = dyn_cast_or_null<Instruction>(U);
   // If not an instruction we do not know how to evaluate.
   // Keep minimum possible cost for now so that it doesnt affect
----------------
Bit of duplication here with the comment above.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:564
 
 /// Compute a bonus for replacing argument \p A with constant \p C.
+void FunctionSpecializer::getSpecializationBonus(Argument *A, Constant *C,
----------------
We are not calculating one bonus anymore, but two values latency and code size.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:583
+    Latency += UserLatency;
+    CodeSize -= UserSize;
   }
----------------
I don't quite get why we are subtracting UserSize here. I guess something related to double or recounting, but after a quick look it  wasn't clear to me, perhaps a comment would be helpful here.


================
Comment at: llvm/test/Transforms/FunctionSpecialization/recursive-penalty.ll:9
+
+; CHECK: FnSpecialization: Created 10 specializations in module
+
----------------
Do we want to check the resulting IR too?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145379