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

[Evan Cheng]

Hi Arnold,

Nitpicks:

I prefer function names which do not require decoding. :-)
How about EmitTailCallLoadRetAddress instead of EmitTCRetAddrLoad?  
Same for EmitTCRetAddrStore.


+  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. 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