[LLVMdev] RFC: PowerPC tail call optimization patch

[Arnold Schwaighofer]

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