thr3ads.net - llvm dev - [LLVMdev] RFC: PowerPC tail call optimization patch [Apr 2008]

If this information is useful, please help other people find it:
Share via:

Arnold Schwaighofer

2008-Apr-22 11:58 UTC

[LLVMdev] RFC: PowerPC tail call optimization patch

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.>
>  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
(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.
>
>       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.
>  +/// GetPossiblePreceedingTailCall - Get preceeding X86ISD::TAILCALL
>  node if it
>  +/// exists skip possible ISD:TokenFactor.
>
>  Comment bug.:) Done

Evan Cheng

2008-Apr-22 20:34 UTC

head link

[LLVMdev] RFC: PowerPC tail call optimization patch

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

Arnold Schwaighofer

2008-Apr-24 17:35 UTC

head link

[LLVMdev] RFC: PowerPC tail call optimization patch

Another round :)

I'll post the patch to llvm-commits cause i guess people might get
annoyed by my series of patches :).

On Tue, Apr 22, 2008 at 10:34 PM, Evan Cheng <evan.cheng at apple.com>
wrote:
.>> +PPCTargetLowering::IsEligibleForTailCallOptimization(SDOperand Call,
>> ...
> That's fine. Please break it into two parts and move the target
> independent part out.Done - see CheckTailCallReturnConstraints() in TargetLowering. The
rest of the function is still target dependent. PowerPC currently can
not tail call optimize byval functions, no pic/got code for non-local
calls. On x86 no support for pic/got code on x86-64 for non-local
calls.
>  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?
>
>Yes the common code between x86/ppc could be moved to
SelectionDAGISel. See CheckDAGForTailCallsAndFixThem().> There seem to be quite a bit of duplicated code. Can you refactor?Created LowerMemOpCallTo.

-arnold

Apparently Analagous Threads

Search for more seemingly similar threads

llvm dev - Apr 2008 - [LLVMdev] RFC: PowerPC tail call optimization patch

[LLVMdev] RFC: PowerPC tail call optimization patch

[LLVMdev] RFC: PowerPC tail call optimization patch

[LLVMdev] RFC: PowerPC tail call optimization patch

Apparently Analagous Threads