[LLVMdev] RFC: PowerPC tail call optimization patch

[Evan Cheng]

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