[llvm-commits] [llvm] r62107 - in /llvm/trunk: lib/Transforms/IPO/Inliner.cpp lib/Transforms/Utils/InlineCost.cpp test/Transforms/Inline/2009-01-12-RecursiveInline.ll

[Dale Johannesen]

On Jan 12, 2009, at 11:57 PMPST, Duncan Sands wrote:

> Hi Dale,
>
>> @@ -180,14 +180,12 @@
>>   Function *Callee = CS.getCalledFunction();
>>   Function *Caller = TheCall->getParent()->getParent();
>>
>> -  // Don't inline a directly recursive call.
>> -  if (Caller == Callee ||
>>       // Don't inline functions which can be redefined at link-time  
>> to mean
>>       // something else.
>>       // FIXME: We allow link-once linkage since in practice all  
>> versions of
>>       // the function have the same body (C++ ODR) - but the LLVM  
>> definition
>>       // of LinkOnceLinkage doesn't require this.
>> -      (Callee->mayBeOverridden() && !Callee->hasLinkOnceLinkage())  
>> ||
>> +   if ((Callee->mayBeOverridden() && !Callee- 
>> >hasLinkOnceLinkage()) ||
>
> the comment (containing the FIXME) is now indented funny.

I don't agree, I think it was indented funny before.  Let's get the  
whole context:

       // Don't inline functions which can be redefined at link-time  
to mean
       // something else.
       // FIXME: We allow link-once linkage since in practice all  
versions of
       // the function have the same body (C++ ODR) - but the LLVM  
definition
       // of LinkOnceLinkage doesn't require this.
    if ((Callee->mayBeOverridden() && !Callee->hasLinkOnceLinkage()) ||
       // Don't inline functions marked noinline.
       Callee->hasFnAttr(Attribute::NoInline) ||  
NeverInline.count(Callee))
     return llvm::InlineCost::getNever();

I think it's better not to put comments into the middle of if  
statements.  But
given that somebody thinks that's a good idea, it makes sense to line  
up the comments
with the part of the if statement they're describing, rather than the  
entire if statement.
This is consistent with what's done with comments after the first.

> Also, it is
> probably OK to inline a function with weak linkage (mayBeOverridden)  
> into
> itself.

Maybe so.  What happens if different compilations make different  
inlining decisions?  Is the linker allowed to require that all copies  
of a weak function are the same?  I think not.