[PATCH] D15401: Refactor threshold computation for inline cost analysis

[Xinliang David Li via llvm-commits]

On Fri, Dec 18, 2015 at 6:07 PM, Chandler Carruth <chandlerc at gmail.com> wrote:
> chandlerc added a comment.
>
> In http://reviews.llvm.org/D15401#312525, @eraman wrote:
>
>> > I'm just suggesting changing the *APIs* to deal in symbolic settings. Clearly the flag will be numeric. I'm suggested sinking the inline threshold flag into the inline cost analysis completely.
>>
>>
>> I attempted to do that and hit an issue. InlineSimple.cpp provides a createFunctionInliningPass((int Threshold) API. To sink thresholds to InlineCost, this needs to be removed, but this is called by LLVMPassManagerBuilderUseInlinerWithThreshold which is exposed by the llvm-c API
>
>
> I had to spend a bunch of time thinking about this.
>
> On one hand, I think exposing this kind of configurability is really frustrating from an API-design perspective. But I think I can imagine users of LLVM (particularly library users) wanting to have pretty wildly different inlining tolerances. However, this raises an important question of how that should be propagated to when the cost analysis has to recurse across yet another function call.

I think we can proceed with the symbolic setting proposals you have
and clean up the main interfaces (to avoid passing threshold).

On the other hand, Easwaran's current patch as it is can already
achieve the goal of recursing across function call -- as the logic of
computing threshold is sinked into call analyzer. The only thing
remaining is the passing the default threshold (which can be user
specified).

>
> I think we need to move *completely* away from having different *initial* thresholds for things like inline-hint and opt-size or min-size. We have numerous adjustments to the threshold based on different analyses properties
. I think inline-hint and size based stuff should work the same way.
This will let you just sink the capping and ballooning of the
threshold into analyzeCall where we also compute all the bonuses. Does
that make sense?

yes.

> It should also avoid the need to separately call 'getInlineThreshold' -- you'll just store and pass along the initial threshold.

Easwaran's current patch does just that -- it only passes along
initial threshold -- the getInlineThreshold is a 'private' method
inside inline cost analysis.

Since Easwaran's current patch is compatible with the longer term goal
(get rid of threshold passing completely) and it is closer to that
goal compared with current state and is a good incremental step.  Do
you have other comments regarding details of the patch?   Incremental
changes like this is much more preferred than more drastic interface
changes.

thanks,

David

David


>
>
> Repository:
>   rL LLVM
>
> http://reviews.llvm.org/D15401
>
>
>