[PATCH] D18560: [TTI] Add getInliningThresholdMultiplier.

[Hal Finkel via llvm-commits]

hfinkel added a subscriber: hfinkel.
hfinkel added a comment.

In http://reviews.llvm.org/D18560#387502, @jlebar wrote:

> In http://reviews.llvm.org/D18560#387488, @eraman wrote:
>
> > Wouldn't it be better to use an additive bonus to the threshold instead of multiplicative factor to account for expensive calls?
>
>
> Not necessarily?  It seems not unreasonable that if we e.g. went from an un-multiplied inlining threshold of 10 to 100 that the adjusted threshold should also increase by 10x, rather than increase by +90.  Like, it depends on what is the intent behind the 10 --> 100 change.
>
> Given how blunt this is, and given that it's used by exactly one target at the moment, I'm inclined not to bikeshed too hard on the exact form it takes.  Like, it would be better to rearrange all of the constants inside InlineCost to better reflect costs on the target machine.  But the discussion of exactly what form this should take would probably be much more productive at the point that what we have doesn't work for some new target.


If you'd like a target-specific cost adjustment, which seems perfectly reasonable, it should be additive, and should be a function of the call site (i.e. both the caller and the callee, not just the caller). Also, hopefully the rationale for these numbers can be clearly articulated in comments in the TTI implementation, so we know how to adjust them if we change how TTI.getUserCost works.


http://reviews.llvm.org/D18560