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

[Xinliang David Li via llvm-commits]

On Tue, Dec 15, 2015 at 10:32 AM, Easwaran Raman <eraman at google.com> wrote:
> eraman added a comment.
>
> In http://reviews.llvm.org/D15401#309175, @davidxl wrote:
>
>> Threshold is used in inline cost analysis to bail out early for compile time reasons, so it is an implementation detail that should not be exposed at interface level. Currently it is needed to be passed because the DefaultThreshold (aka callSiteIndependent Threshold) depends on opt level, sizeOptLevel which ICA does not have access to.
>>
>> The solution is pretty simple:  just pass the optlevel to ICA when ICA is created.   If Threshold does need to be passed for some reason (which I doubt) please rename such parameters (including the member field in SimpleInliner) to be DefaultThreshold (with comment it depends on opt level) to avoid confusion. CallsiteIndependentThreshold is too long a name to use.
>
>
> SimpleInliner instances are created through createFunctionInliningPass. There are three overloads of this method. One passes the size and opt level, but the other two either explicitly passes the threshold (this is used in bugpoint) or uses a default threshold. So there is no way to avoid passing the threshold. I will do the renaming.
>

Ok.

>
> ================
> Comment at: include/llvm/Analysis/InlineCost.h:153
> @@ -141,3 +152,3 @@
>  }
>
>  #endif
> ----------------
> Unfortunately, there is subtle difference in how the threshold is computed for -Os and OptimizeForSize attribute. If -Os is specified, the default threshold is lowered to 75. This is independent of whether we pass -inline-threshold or not. However, if -Os is not specified and the caller function has  OptimizeForSize attribute, the threshold is lowered only if -inline-threshold is not specified.

Worth cleanup in a different patch..


>
> Additionally if -Oz is specified, the threshold is lowered to 25, but the getInlineThreshold does not check for MinSize attribute at all.
>
> Clearly, this needs to be fixed to give a consistent behavior but I think should be done separately.

Agreed.

thanks,

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