[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

[Chris Lattner]

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.


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

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

> -      // 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();

-Chris