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-16 17:07 UTC

[LLVMdev] RFC: PowerPC tail call optimization patch

Hello Dale,

this is an updated version of the tail call optimization patch for
powerpc. could you have a look at it?

i added code to support ppc64 (untested, will try to get access to
ppc64 on a friend's machine).
incorporated evan's formatting suggestions. ;)

will run another round of testing (llvm-test) on my powerpc g4/800
when i get the okay to commit. testing on this machine takes forever
:(.

okay to commit?

regards arnold
On Fri, Mar 28, 2008 at 12:01 AM, Evan Cheng <evan.cheng at apple.com>
wrote:>
>
>  On Mar 26, 2008, at 11:29 PM, Arnold Schwaighofer wrote:
>
>  > Hi there,
>  >
>  > it's me again. attached is preliminary support for tail calls on
ppc
>  > 32. (once i get access to a 64bit machine that part will follow). It
>  > has passed the llvm-test with the -tailcallopt flag enabled. (after
>  > half a day on an old g4/800 :)
>  >
>  > okay to submit? probably not. :) suggestions.
>
>  I can see some stylistic issues. e.g. getFramePointerFrameIndex() etc.
>  should be declared const, also please make sure there is a space
>  between 'if' and '('. But I don't enough about PPC to
really pick on
>  it. :-)
>
>  Dale, can you look through the patch?-------------- next part --------------
A non-text attachment was scrubbed...
Name: r49787-ppc-1.patch
Type: text/x-patch
Size: 64944 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20080416/d764ffbe/attachment.bin>

Evan Cheng

2008-Apr-21 22:30 UTC

head link

[LLVMdev] RFC: PowerPC tail call optimization patch

On Apr 16, 2008, at 10:07 AM, Arnold Schwaighofer wrote:
> Hello Dale,
>
> this is an updated version of the tail call optimization patch for
> powerpc. could you have a look at it?
>
> i added code to support ppc64 (untested, will try to get access to
> ppc64 on a friend's machine).
> incorporated evan's formatting suggestions. ;)
>
> will run another round of testing (llvm-test) on my powerpc g4/800
> when i get the okay to commit. testing on this machine takes forever
> :(.
More nitpicks:

+      if (!isVarArg && !isPPC64) {
+        // Non-varargs Altivec parameters go after all the non-Altivec
+        // parameters; handle those later so we know how much padding  
we need.
+        nAltivecParamsAtEnd++;
+        continue;
+      } else {
+        // Varargs and 64-bit Altivec parameters are padded to 16  
byte boundary.
+        NumBytes = ((NumBytes+15)/16)*16;
+      }

=>

+      if (!isVarArg && !isPPC64) {
+        // Non-varargs Altivec parameters go after all the non-Altivec
+        // parameters; handle those later so we know how much padding  
we need.
+        nAltivecParamsAtEnd++;
+        continue;
+      }
+      // Varargs and 64-bit Altivec parameters are padded to 16 byte  
boundary.
+      NumBytes = ((NumBytes+15)/16)*16;

No need for else here. :-)


+  int SPDiff = 0;
+
+  PPCFunctionInfo *FI =  
DAG.getMachineFunction().getInfo<PPCFunctionInfo>();
+  unsigned CallerMinReservedArea = FI->getMinReservedArea();
+  SPDiff = (int)CallerMinReservedArea - (int)ParamSize;

Just change last statement to
int SPDiff = (int)...


+bool
+PPCTargetLowering::IsEligibleForTailCallOptimization(SDOperand Call,
+                                                     SDOperand Ret,
+                                                     SelectionDAG&  
DAG) const {
+  // Variable argument functions
+  // are not supported.

Why two separate lines?

+  if (!PerformTailCallOpt ||
+      cast<ConstantSDNode>(Call.getOperand(2))->getValue() != 0)  
return false;
+
+
+
+

So many blank lines... :-)


+  // 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)?

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?


      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?

+/// GetPossiblePreceedingTailCall - Get preceeding X86ISD::TAILCALL  
node if it
+/// exists skip possible ISD:TokenFactor.

Comment bug.

Thanks,

Evan

>
>
> okay to commit?
>
> regards arnold
> On Fri, Mar 28, 2008 at 12:01 AM, Evan Cheng <evan.cheng at
apple.com>
> wrote:
>>
>>
>> On Mar 26, 2008, at 11:29 PM, Arnold Schwaighofer wrote:
>>
>>> Hi there,
>>>
>>> it's me again. attached is preliminary support for tail calls
on ppc
>>> 32. (once i get access to a 64bit machine that part will follow).
It
>>> has passed the llvm-test with the -tailcallopt flag enabled. (after
>>> half a day on an old g4/800 :)
>>>
>>> okay to submit? probably not. :) suggestions.
>>
>> I can see some stylistic issues. e.g. getFramePointerFrameIndex()  
>> etc.
>> should be declared const, also please make sure there is a space
>> between 'if' and '('. But I don't enough about PPC
to really pick on
>> it. :-)
>>
>> Dale, can you look through the patch?
> <r49787-ppc-1.patch>_______________________________________________
> 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-22 11:58 UTC

head link

[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

Possibly Parallel Threads

Search for more possibly parallel 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

Possibly Parallel Threads