[llvm-commits] [llvm] r165367 - /llvm/trunk/lib/Analysis/InlineCost.cpp

[Bob Wilson]

On Oct 6, 2012, at 7:11 PM, Bob Wilson <bob.wilson at apple.com> wrote:

> 
> On Oct 6, 2012, at 7:02 PM, Chandler Carruth <chandlerc at google.com> wrote:
> 
>> On Sat, Oct 6, 2012 at 6:11 PM, Bob Wilson <bob.wilson at apple.com> wrote:
>> Author: bwilson
>> Date: Sat Oct  6 20:11:19 2012
>> New Revision: 165367
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=165367&view=rev
>> Log:
>> Make sure always-inline functions get inlined. <rdar://problem/12423986>
>> 
>> Without this change, when the estimated cost for inlining a function with
>> an "alwaysinline" attribute was lower than the inlining threshold, the
>> getInlineCost function was returning that estimated cost rather than the
>> special InlineCost::AlwaysInlineCost value. That is fine in the normal
>> inlining case, but it can fail when the inliner considers the opportunity
>> cost of inlining into an internal or linkonce-odr function. It may decide
>> not to inline the always-inline function in that case. The fix here is just
>> to make getInlineCost always return the special value for always-inline
>> functions. I ran into this building clang with libc++. Tablegen failed to
>> link because of an always-inline function that was not inlined. I have been
>> unable to reduce the testcase down to a reasonable size.
>> 
>> This is a bit of a hack though, and it makes the rest of the inline cost analysis not make very much sense. If we're never going to even use the estimated cost when there is an always-inline attribute, why are we computing it? I never liked the way always inline was handled here, and it feels like this is a band-aid over the real symptom.
> 
> I barely know this code, but it doesn't seem like a hack to me.

To elaborate on this, consider 2 different functions marked with the alwaysinline attribute: one of them has an estimated cost just below the threshold and the other is just above the threshold.  I don't remember what the threshold value is, but hypothetically, let's say it is 200.  One function's cost is 199 and the other is 201.  Without my patch, getInlineCost would return 199 for the first function and INT_MIN (InlineCost::AlwaysInlineCost) for the other.  That makes no sense.  With my patch, they both get an estimated cost of INT_MIN.

> However, I agree about the rest of it not making much sense.  I kept wondering why we were going to so much effort to estimate the costs.  I assumed it was because we need to scan all the code to check for things like recursive calls that would prevent inlining.  Running the always-inline pass earlier would have the same effect and would let us simplify the inline cost calculation.  My only concern would be a possible impact on compile time.

After thinking about it a bit, my recommendation would be to keep the passes as they are, but change InlineCost::getInlineCost to check for alwaysinline attributes as a special case and use the same code as AlwaysInliner::getInlineCost to make sure it is viable to inline.  That would also allow removing all the special cases for AlwaysInline in the CallAnalyzer.

> 
> 
>> 
>> What do you think about instead having the pass manager run the always-inliner pass much earlier in the compilation? That's what we do for -O0, and I feel like all of InlineCost would be simpler if we could assert that it never runs for functions attributed as always-inline.
>>  
>> 
>> Modified:
>>     llvm/trunk/lib/Analysis/InlineCost.cpp
>> 
>> Modified: llvm/trunk/lib/Analysis/InlineCost.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/InlineCost.cpp?rev=165367&r1=165366&r2=165367&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Analysis/InlineCost.cpp (original)
>> +++ llvm/trunk/lib/Analysis/InlineCost.cpp Sat Oct  6 20:11:19 2012
>> @@ -142,6 +142,7 @@
>> 
>>    int getThreshold() { return Threshold; }
>>    int getCost() { return Cost; }
>> +  bool isAlwaysInline() { return AlwaysInline; }
>> 
>>    // Keep a bunch of stats about the cost savings found so we can print them
>>    // out when debugging.
>> @@ -1057,7 +1058,8 @@
>>    // Check if there was a reason to force inlining or no inlining.
>>    if (!ShouldInline && CA.getCost() < CA.getThreshold())
>>      return InlineCost::getNever();
>> -  if (ShouldInline && CA.getCost() >= CA.getThreshold())
>> +  if (ShouldInline && (CA.isAlwaysInline() ||
>> +                       CA.getCost() >= CA.getThreshold()))
>>      return InlineCost::getAlways();
>> 
>>    return llvm::InlineCost::get(CA.getCost(), CA.getThreshold());
>> 
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121007/55d3f0f2/attachment.html>