[llvm-commits] PATCH: Tailcallopt x86 byval argument handling

[Arnold Schwaighofer]

On Tue, Apr 8, 2008 at 11:38 PM, Evan Cheng <evan.cheng at apple.com> wrote:
> Hi Arnold,
>
>  Nitpicks:
>
>  I prefer function names which do not require decoding. :-)
>  How about EmitTailCallLoadRetAddress instead of EmitTCRetAddrLoad?
>  Same for EmitTCRetAddrStore.

Hmm agreed. Somehow i got tempted :).

>  +  MVT::ValueType VT = Is64Bit ? MVT::i64 : MVT::i32;
>  +  SDOperand NewRetAddrFrIdx = DAG.getFrameIndex(NewReturnAddrFI, VT);
>  Please use getPointerTy() instead.
>
>  +bool
>  +X86TargetLowering::CopyByValClobberedRegToVirtReg(bool
>  containsByValArg,
>  +                                      int CntUsedVRegsByVal,
>  +                                      unsigned VRegsByVal [3],
>  +                                      unsigned VRegsByValDest [3],
>  +                                      MVT::ValueType VRegsByValVTs [3],
>  +                                      std::pair<unsigned, SDOperand>
>  &RegToPass,
>  +                                      SDOperand &OutChain,
>  +                                      SDOperand &OutFlag,
>  +                                      MachineFunction &MF,
>  +                                      SelectionDAG & DAG) {
>  +  if (!containsByValArg || !(RegToPass.first == X86::RSI ||
>  +                           RegToPass.first == X86::RDI ||
>  +                           RegToPass.first == X86::ESI ||
>  +                           RegToPass.first == X86::EDI ||
>  +                           RegToPass.first == X86::ECX ||
>  +                           RegToPass.first == X86::RCX)) return false;
>
>  I am not very comfortable with this. Is the registers that can be
>  impacted somehow specified in X86CallingConv.td? I don't want to get
>  into a situation where someone updates the .td file but forgot to
>  update this code.
I am not sure i understand. The impacted Registers RSI,RDI,RCX (or
their 32bit respectives) always stay the same. They have nothing to do
with the callingconvetion. A change in callling convention - which
registers are used for passing does not influence above set.
Above set is cause by the fact that the following sequence is
neccesary for tail calls.
forall actParams as ArgI

move ArgI to
> Will this work for all the CC that might want tail
>  call support?
>
>  /// RestoreByValClobberedReg - Restore registers which were
>  overwritten by byval
>  +/// tail call lowering from virtual registers.
>  +static SDOperand RestoreByValClobberedReg(SelectionDAG & DAG,
>  SDOperand Chain,
>  +                                          int CntUsedVRegsByVal,
>  +                                          unsigned VRegsByVal [3],
>  +                                          unsigned VRegsByValDest [3],
>  +                                          MVT::ValueType
>  VRegsByValVTs [3]) {
>  +  if (!CntUsedVRegsByVal) return Chain;
>  +  SDOperand InFlag;
>  +  while (CntUsedVRegsByVal) {
>  +    CntUsedVRegsByVal--;
>  +    SDOperand Tmp = DAG.getCopyFromReg(Chain,
>  VRegsByVal[CntUsedVRegsByVal],
>  +
>  VRegsByValVTs[CntUsedVRegsByVal],
>  +                                       InFlag);
>  +    Chain = DAG.getCopyToReg(Chain, VRegsByValDest[CntUsedVRegsByVal],
>  +                             Tmp, InFlag);
>  +    InFlag = Chain.getValue(1);
>  +  }
>  +  return Chain;
>  +}
>
>  Is it necessary for all the copyfromreg and copytoreg to be chained
>  and flagged together? Perhaps each pair need to be flagged together
>  and chains from all pairs ended up feeding into a TokenFactor?
>
>  +  unsigned VRegsByVal [3];
>
>  Why the space between VRegsByVal and '['? :-)
>
>  +        // Remember fact that this call contains byval arguments.
>  +        if (!containsByValArg && IsTailCall && isByVal)
>  containsByValArg=true;
>  =>
>  containsByValArg |= isTailCall && isByVal;
>  ?
>
>  +        MVT::ValueType VT = Is64Bit ? MVT::i64 : MVT::i32;
>           FI = MF.getFrameInfo()->CreateFixedObject(OpSize, Offset);
>  -        FIN = DAG.getFrameIndex(FI, MVT::i32);
>  +        FIN = DAG.getFrameIndex(FI, VT);
>
>  getPointerTy().
>
>  Thanks,
>
>  Evan
>
>
>
>
>
>  On Apr 7, 2008, at 3:17 PM, Arnold Schwaighofer wrote:
>
>  > Hi Evan (and everyone else who wants to bash this patch :),
>  >
>  > this patch corrects handling of byval arguments for tailcall optimized
>  > x86-64 (and x86) calls so that they work (... at least for my test
>  > cases).
>  >
>  > Should fix the following problems:
>  >
>  > Problem 1: when i introduced the optimized handling of arguments for
>  > tail called functions (not always moving them to a spill slot -
>  > actually moving to the destination of the arg if the call were to be a
>  > normal function call - before storing them at their 'tailcall'
>  > position but using virtual registers) i did not handle byval arguments
>  > correctly.
>  >
>  > Problem 2: on x86-64 after the arguments of the tail called function
>  > are moved to their registers (which include ESI/RSI etc), tail call
>  > optimization performs byval lowering which causes xSI,xDI, xCX
>  > registers to be overwritten. this is handled in this patch by moving
>  > the arguments to virtual registers first and after the byval lowering
>  > the arguments are moved from those virtual registers back to
>  > RSI/RDI/RCX.
>  >
>  > Okay to commit?
>  >
>  > regards arnold
>  > <x86-byval.patch>_______________________________________________
>  > llvm-commits mailing list
>  > llvm-commits at cs.uiuc.edu
>  > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>  _______________________________________________
>  llvm-commits mailing list
>  llvm-commits at cs.uiuc.edu
>  http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>