[llvm-commits] [llvm] r48020 - in /llvm/trunk: lib/Transforms/Utils/InlineFunction.cpp test/Transforms/SRETPromotion/2008-03-07-Inline-2.ll test/Transforms/SRETPromotion/2008-03-07-Inline.ll

[Devang Patel]

On Mar 7, 2008, at 5:19 PM, Chris Lattner wrote:

> On Mar 7, 2008, at 12:06 PM, Devang Patel wrote:
>> URL: http://llvm.org/viewvc/llvm-project?rev=48020&view=rev
>> Log:
>> Update inliner to handle functions that return multiple values.
>
> Nice.
>
>> @@ -0,0 +1,46 @@
>> +; RUN: llvm-as < %s | opt -inline -sretpromotion -disable-output
>
> Again, your test should only be for the inliner, not both passes.

ok

>> =
>> =
>> =====================================================================
>> --- llvm/trunk/lib/Transforms/Utils/InlineFunction.cpp (original)
>> +++ llvm/trunk/lib/Transforms/Utils/InlineFunction.cpp Fri Mar  7
>> 14:06:16 2008
>>
>>  // Handle all of the return instructions that we just cloned in,
>> and eliminate
>>  // any users of the original call/invoke instruction.
>> -  if (Returns.size() > 1) {
>> +  if (!Returns.empty()) {
>
> You are (apparently, I may be mistaken) dropping an important
> optimization here.

You're not mistaken. I expect simplification pass to merge the block  
easily, but did not realize that extra br and one additional block can  
have significant impact on inlining cost. I'll restore this.

> When a function only has a single return, we don't
> want the inliner to turn:
>
> ...
> call foo()
> ...
>
> into:
>
> ...
> somestuff
> br label x
>
> x:
> ...
>
> This increases the apparent inlining cost of the resultant code and is
> slow.  Please don't lose this.  The trick here is that you can't
> insert a phi node into the middle of the block, even if it's a single
> entry one.  Not doing this optimization for multiple return calls
> would be fine.
>
>>    // The PHI node should go at the front of the new basic block to
>> merge all
>>    // possible incoming values.
>> +    SmallVector<PHINode *, 4> PHIs;
>>    if (!TheCall->use_empty()) {
>> +      const Type *RTy = CalledFunc->getReturnType();
>> +      if (const StructType *STy = dyn_cast<StructType>(RTy)) {
>> +        unsigned NumRetVals = STy->getNumElements();
>> +        // Create new phi nodes such that phi node number in the
>> PHIs vector
>> +        // match corresponding return value operand number.
>> +        for (unsigned i = 0; i < NumRetVals; ++i) {
>> +          PHINode *PHI = new PHINode(STy->getElementType(i),
>> +                                     TheCall->getName(),
>> AfterCallBB->begin());
>
> I'd suggest naming these things 'TheCall->getName()+"."+utostr(i)'.

ok

> Also, you're inserting the phi nodes in backwards order (the block
> will have rv3,rv2,rv1,rv0 in that order) which is not a functionality
> issue but will make the ir be a bit strange looking.

I already noticed that :) I'll restore natural order.

>
>
>>
>> +          PHIs.push_back(PHI);
>> +        }
>> +        // TheCall results are used by GetResult instructions.
>> +        while (!TheCall->use_empty()) {
>> +          GetResultInst *GR = cast<GetResultInst>(TheCall-
>>> use_back());
>> +          GR->replaceAllUsesWith(PHIs[GR->getIndex()]);
>> +          GR->eraseFromParent();
>
> Nice.
>
>> +    // Loop over all of the return instructions adding entries to
>> the PHI node as
>>    // appropriate.
>> +    if (!PHIs.empty()) {
>> +      const Type *RTy = CalledFunc->getReturnType();
>> +      if (const StructType *STy = dyn_cast<StructType>(RTy)) {
>
> I don't think you need to distinguish between the multiple return and
> scalar return case here.  Just loop over all the elements of Returns,
> and then loop over all their operands.

ok

>> -      // Add a branch to the merge point where the PHI node lives
>> if it exists.
>> +    // Add a branch to the merge points and remove retrun
>> instructions.
>> +    for (unsigned i = 0, e = Returns.size(); i != e; ++i) {
>> +      ReturnInst *RI = Returns[i];
>>      new BranchInst(AfterCallBB, RI);
>>      RI->getParent()->getInstList().erase(RI);
>>    }
>
> How about RI->eraseFromParent();

good idea!
-
Devang