[PATCH] D28369: Refactor inline threshold update code.

[Xinliang David Li via llvm-commits]

On Fri, Jan 6, 2017 at 3:37 PM, Easwaran Raman via Phabricator <
reviews at reviews.llvm.org> wrote:

> eraman added a comment.
>
> In https://reviews.llvm.org/D28369#638393, @davidxl wrote:
>
> > Should we honor optForMinSize for HotCallsite case as well?
>
>
> Yes, unless and until someone tunes PGO + -Os case.
>
> > I also suggest split the source hint based check from the profile driven
> check.  Also check hotcallsite first:
> >
> > if (!Caller->optForMinSize() ) {
> >
> >   if (...InlineHint)
> >     Threshold = MaxIfValid (...);
> >
> >   if (HotCallsite)
> >      ...
> >   else if (HotCallee)
> >     ..
> >
> > }
>
> Note that HotCallsite and HotCallee are mutually exclusive and if the
> callsite is hot that takes precedence over HotCallee. So
> your comment on "check hot callsite first" is already done.



yes -- but the way it is written it is a little subtle. I think it is
better to make the precedence explicit.



> I also don't see much point in writing
>
> if (Callee.hasFnAttribute(Attribute::InlineHint))
>
>   Threshold = MaxIfValid(Threshold, Params.HintThreshold);
>
> if ( HotCallee)
>
>   Threshold = MaxIfValid(Threshold, Params.HintThreshold);
>
> instead of the current
>
> if (Callee.hasFnAttribute(Attribute::InlineHint) || HotCallee)
>
>   Threshold = MaxIfValid(Threshold, Params.HintThreshold);
>
>
It is fine as it is, but I think the suggested way does allow us to split
HotCallee with a different threshold parameter in the future if needed. Tie
it with InlineHint does not sound the right thing longer term.

David



>
> https://reviews.llvm.org/D28369
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170106/b625aa6e/attachment.html>