[PATCH] D24338: [InlineCost] Remove CallPenalty and change MinSizeThreshold to 5

[Xinliang David Li via llvm-commits]

On Tue, Nov 1, 2016 at 10:39 PM, Mehdi Amini <mehdi.amini at apple.com> wrote:

>
> On Nov 1, 2016, at 10:01 PM, Xinliang David Li <davidxl at google.com> wrote:
>
>
>
> On Tue, Nov 1, 2016 at 8:06 PM, Mehdi AMINI <mehdi.amini at apple.com> wrote:
>
>> mehdi_amini added a comment.
>>
>> (Add inline the answer to David, so that there is the relevant context)
>>
>>
>>
>> ================
>> Comment at: lib/Analysis/InlineCost.cpp:938
>> -      if (!isa<InlineAsm>(CS.getCalledValue()))
>> -        Cost += InlineConstants::CallPenalty;
>>      }
>> ----------------
>> >>! In D24338, @davidxl wrote:
>> > Do you mean parameter passing? That should be counted independently  by
>> the instruction visitor.
>>
>> No, I don’t mean the parameters, the visitor account for one instruction
>> per parameter just before this indeed.
>> However a call has other extra-cost on top of a regular instruction, even
>> without any argument: it splits live-ranges and may cause spills around it
>> to preserve registers that aren't callee-saved.
>>
>> In this sense, it make sense that a call can "cost" more than a normal
>> instruction.
>>
>
> yes, inlining can lead to increased register pressure leading to use of
> caller save registers (and spills around new callsites). However I think
> this should not be blindly applied -- better tied with register pressure
> analysis.
>
>
> I’m not sure we’re talking on the same level here: I don’t claim the the
> register pressure will increase in the caller after inlining here, or at
> least not because of call instructions in the callee.
> My assumption (possibly incorrect), is that the current inliner cost model
> trades off the cost of the call site with the increased code size resulting
> from the inlining.
>
> When we consider the overall “cost” of a callee: we a cost per-instruction
> in the visitor. The question is what is this cost representing? From a code
> size point of view, it make sense to add a penalty for call instructions
> because they have potentially more impacts (spills is an example that is
> specific to calls).
>

>From the way it is currently implemented, it is likely the cost is to model
the size increase. Yes, spills can result in size increase too -- but my
point is that size cost of spills can not be unconditionally applied.

To compute runtime cost/benefit, the 'runtime cost diff's of the callee and
inline instance of the callee needs to be computed (with the help of
profile info). Easwaran has a speed-up analysis patch that does that.

>
> I don’t believe the patch as-is demonstrate that removing this penalty is
> the right thing to do: I’d like to see first how it compares to:
> 1) simply starting by deducing the cost of one call (the call-site
> considered for inlining) ;
>
it is included in this patch, but should be split and committed first:
> that’s mostly a “bug fix”.
>

this sounds reasonable.


> 2) simply increasing the threshold.
>
> This is a different tuning IMO.


> Without these additional baseline, it is not clear to me that the
> measurement is sound: we can very well obtain better result because we will
> now inline a function that has multiple calls, while there can exist
> similar function without many calls but a bit more arithmetic that would
> also be profitable to inline.
>

Or do this: split the current CallPenalty into two parameters:
* CallPenalty which is used in patch  #1 you suggested -- i.e, the value to
be deducted from the cost of  callsite to be inlined
* CallInstrCost -- this value is used to model the 'size cost' of call
instruction (which considers potential spilling code). The value can be
tuned independently -- lowed below 25 but higher than 5 (the default
instruction cost).

David


> —
> Mehdi
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161101/ed53c71a/attachment.html>