[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:30 PM, Devang Patel wrote:

>
> 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

done

>
>
>>> =
>>> =
>>> =
>>> ====================================================================
>>> --- 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.

done

>
>
>> 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

done

>
>
>> 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.

done

>
>
>>
>>
>>>
>>> +          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

done
>
>
>>> -      // 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!

done

>
> -
> Devang
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-
Devang