[LLVMdev] RFC: PowerPC tail call optimization patch

[Evan Cheng]

On Apr 22, 2008, at 4:58 AM, Arnold Schwaighofer wrote:

> On Tue, Apr 22, 2008 at 12:30 AM, Evan Cheng <evan.cheng at apple.com>  
> wrote:
>> More nitpicks:
>> ...
>> No need for else here. :-)
> Done
>> SPDiff = (int)CallerMinReservedArea - (int)ParamSize;
>>
>> Just change last statement to
>> int SPDiff = (int)...
> Done
>>
>> +bool
>> +PPCTargetLowering::IsEligibleForTailCallOptimization(SDOperand Call,
>> +                                                     SDOperand Ret,
>> +                                                     SelectionDAG&
>> DAG) const {
>> +  // Variable argument functions
>> +  // are not supported.
>>
>> Why two separate lines?
> there once was some text ... done.
>
>> So many blank lines... :-)
> done :)
>
> you wouldn't have by any chance an evan awk script that checks for my
> stupid fomatting mistakes :)

:-)

>
>
>> +  // Check whether CALL node immediatly preceeds the RET node and
>> whether the
>> +  // return uses the result of the node or is a void return.
>> +  unsigned NumOps = Ret.getNumOperands();
>> +  if ((NumOps == 1 &&
>> +       (Ret.getOperand(0) == SDOperand(Call.Val,1) ||
>> +        Ret.getOperand(0) == SDOperand(Call.Val,0))) ||
>> +      (NumOps > 1 &&
>> +       Ret.getOperand(0) == SDOperand(Call.Val,Call.Val-
>>> getNumValues()-1) &&
>> +       Ret.getOperand(1) == SDOperand(Call.Val,0))) {
>> ...
>>
>> This function seems to be very similar to
>> X86TargetLowering::IsEligibleForTailCallOptimization. Is it possible
>> to make it into a common utility function (or at least the target
>> independent part)?
> The target independent part can be refactored. The whole function not
> because e.g ppc does not support yet fully support PIC.

That's fine. Please break it into two parts and move the target  
independent part out.

>
>>
>> Also
>> +static bool IsPossiblyOverwrittenArgumentOfTailCall(SDOperand Op,
>> +                                                    MachineFrameInfo
>> * MFI) {
>>
>> I wonder if these can be moved to SelectionDAGISel? So it's handled
>> during the sdisel phase of call lowering?
>
> I am not sure what you mean by 'these' here. The function above

IsEligibleForTailCallOptimization,  
IsPossiblyOverwrittenArgumentOfTailCall, etc. Basically anything  
that's shared between all the targets.

>
> (IsPossiblyOverwritten...) is used in
> LowerCALL()
>  forall arguments
>    CalculateTailCallArgDest()
> to determine whether an argument needs to be evacuated to a virtual
> register to prevent being overwritten.
>
> I am not sure what you are suggesting ? Storing this information
> (ispossiblyoverwritten) in the argument nodes?
> as ARG_FLAGSSDNode as a new ArgFlagsTy?
>
> You are using 'these'. I believe: the other functions e.g
> CalculateTailCallArgDest should not be moved to SelectionDAGISel
> because they involve target knowledge (where to place the argument)
> and would involve moving the whole argument lowering to
> SelectionDAGISel.

I am not sure. Just thinking aloud. It would be nice if all of the  
common code can be handled by SelectionDAGISel.  For example, it seems  
possible for SDIsel to determine which operands can be overwritten and  
issued the copies there?

>
>
>>
>>      if (isPPC64 && Arg.getValueType() == MVT::i32) {
>>        // FIXME: Should this use ANY_EXTEND if neither sext nor zext?
>> @@ -1946,7 +2285,13 @@ SDOperand
>> PPCTargetLowering::LowerCALL(SDOperand Op, SelectionDAG &DAG,
>>        if (GPR_idx != NumGPRs) {
>>          RegsToPass.push_back(std::make_pair(GPR[GPR_idx++], Arg));
>>        } else {
>> -        MemOpChains.push_back(DAG.getStore(Chain, Arg, PtrOff, NULL,
>> 0));
>> +        if (!isTailCall)
>> +          MemOpChains.push_back(DAG.getStore(Chain, Arg, PtrOff,
>> NULL, 0));
>> +        // Calculate and remember argument location.
>> +        else
>> +          CalculateTailCallArgDest(DAG, MF, isPPC64, Arg, SPDiff,
>> ArgOffset,
>> +                                   TailCallArguments);
>> +
>>          inMem = true;
>>        }
>>        if (inMem || isMachoABI) {
>> @@ -1994,7 +2339,13 @@ SDOperand
>> PPCTargetLowering::LowerCALL(SDOperand Op, SelectionDAG &DAG,
>>            }
>>          }
>>        } else {
>> -        MemOpChains.push_back(DAG.getStore(Chain, Arg, PtrOff, NULL,
>> 0));
>> +        if (!isTailCall)
>> +          MemOpChains.push_back(DAG.getStore(Chain, Arg, PtrOff,
>> NULL, 0));
>> +         // Calculate and remember argument location.
>> +        else
>> +          CalculateTailCallArgDest(DAG, MF, isPPC64, Arg, SPDiff,
>> ArgOffset,
>> +                                   TailCallArguments);
>> +
>> ...
>> -        // We are emitting Altivec params in order.
>> -        PtrOff = DAG.getNode(ISD::ADD, PtrVT, StackPtr,
>> -                            DAG.getConstant(ArgOffset, PtrVT));
>> -        SDOperand Store = DAG.getStore(Chain, Arg, PtrOff, NULL, 0);
>> -        MemOpChains.push_back(Store);
>> +        if (!isTailCall) {
>> +          // We are emitting Altivec params in order.
>> +          PtrOff = DAG.getNode(ISD::ADD, PtrVT, StackPtr,
>> +                               DAG.getConstant(ArgOffset, PtrVT));
>> +          SDOperand Store = DAG.getStore(Chain, Arg, PtrOff, NULL,  
>> 0);
>> +          MemOpChains.push_back(Store);
>> +        } else {
>> +          CalculateTailCallArgDest(DAG, MF, isPPC64, Arg, SPDiff,
>> ArgOffset,
>> +                                   TailCallArguments);
>> +        }
>>
>> There seem to be quite a bit of duplicated code. Can you refactor?
>
> Will try to refactor into one function performing above functionality
> that is called instead.

Thanks.

Evan

>
>
>> +/// GetPossiblePreceedingTailCall - Get preceeding X86ISD::TAILCALL
>> node if it
>> +/// exists skip possible ISD:TokenFactor.
>>
>> Comment bug.
> :) Done
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev