[PATCH] D119880: [FuncSpec] Support function specialization across multiple arguments.

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.
Herald added a project: All.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:550
+    // instructions in the function.
+    return Metrics.NumInsts * InlineConstants::InstrCost;
   }
----------------
labrinea wrote:
> SjoerdMeijer wrote:
> > This was fundamental how the cost model used to work: the more function specialised, the harder that become due to the Penalty. So I disagree with this statement:
> > 
> > >  I've also lifted two arbitrary limitations around the specialization selection: the penalty in cost estimation ....
> > 
> > This was not arbitrary, but by design. 
> > 
> > 
> Maybe "arbitrary" was the wrong vocabulary here. Sorry. What I mean is that it is not fair to bias the calculation of the cost of a given function based on historical data as "how many specializations have happend so far". As I explained on the description, a potential specialization may never trigger even if it is more profitable from one that did, just because it was discovered first. I could separate this change to another patch if it makes the review easier.
Ok, I see, I probably didn't get that idea, but it makes sense. I guess the general idea is to collect all candidates first, calculate a profitability, then select a few candidates. Maybe I am still expecting some sort of penalty the more gets specialised. But definitely, if you can split more things off, that would certainly help. This needs a rebase anyway I guess, and I need to look at this again after that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119880