xcore: handling of byval args in callee

Richard Osborne richard at xmos.com
Thu Jul 25 14:21:46 PDT 2013


Hi Robert,

Sorry for not getting around to reviewing this sooner, comments inline. Would you be able to supply an updated patch?

Thanks,

Richard

On 19 Jul 2013, at 15:07, Robert Lytton <robert at xmos.com<mailto:robert at xmos.com>> wrote:

Hi Richard,

Here is a patch for handling byval arguments for the xcore ABI.

robert

<patch>


diff --git a/lib/Target/XCore/XCoreISelLowering.cpp b/lib/Target/XCore/XCoreISelLowering.cpp
index 5f3d935..422fef3
--- a/lib/Target/XCore/XCoreISelLowering.cpp
+++ b/lib/Target/XCore/XCoreISelLowering.cpp
@@ -1031,6 +1031,8 @@ XCoreTargetLowering::LowerCallResult(SDValue Chain, SDValue InFlag,
 //             Formal Arguments Calling Convention Implementation
 //===----------------------------------------------------------------------===//



+struct ByValPair{SDValue SDV; ISD::ArgFlagsTy Flags;};
+

Local types should be placed in an anonymous namespace.

 /// XCore formal arguments implementation
 SDValue
 XCoreTargetLowering::LowerFormalArguments(SDValue Chain,
@@ -1080,9 +1082,20 @@ XCoreTargetLowering::LowerCCCArguments(SDValue Chain,



   unsigned LRSaveSize = StackSlotSize;



+  // all getMemcpys must follow getCopyFromReg to prevent clobbering
+  // 1. CopyFromReg (and load) arg & vararg registers
+  // 2. chain CopyFromReg nodes into a TokenFactor
+  // 3. Memcpy any 'byVal' args
+  // 4, chain all the mem ops nodes into a TokenFactor
+  SmallVector<SDValue, 4> RegNode;
+  SmallVector<ByValPair, 4> BVData;
+  SmallVector<SDValue, 4> MemOps;
+
+  // 1a. CopyFromReg (and load) arg registers
   for (unsigned i = 0, e = ArgLocs.size(); i != e; ++i) {



     CCValAssign &VA = ArgLocs[i];
+    ISD::ArgFlagsTy Flags = Ins[i].Flags;



     if (VA.isRegLoc()) {
       // Arguments passed in registers
@@ -1099,7 +1112,15 @@ XCoreTargetLowering::LowerCCCArguments(SDValue Chain,
       case MVT::i32:
         unsigned VReg = RegInfo.createVirtualRegister(&XCore::GRRegsRegClass);
         RegInfo.addLiveIn(VA.getLocReg(), VReg);
-        InVals.push_back(DAG.getCopyFromReg(Chain, dl, VReg, RegVT));
+        SDValue CFRN = DAG.getCopyFromReg(Chain, dl, VReg, RegVT);
+        RegNode.push_back(CFRN.getValue(CFRN->getNumValues()-1));
+        if (Flags.isByVal() && Flags.getByValSize()) {
+          // delay creating copy of byval argument & adding result to InVals
+          const ByValPair BVD = {CFRN,Flags};
+          BVData.push_back(BVD);
+        } else {
+          InVals.push_back(CFRN);
+        }

This doesn't seem right. If I have a function such as void f(struct foo a, int b) then this will push the copyFromReg for b into InVals before it inserting the value for the byval structure into InVals, leaving InVals in the wrong order.
       }
     } else {
       // sanity check
@@ -1119,12 +1140,20 @@ XCoreTargetLowering::LowerCCCArguments(SDValue Chain,
       // Create the SelectionDAG nodes corresponding to a load
       //from this parameter
       SDValue FIN = DAG.getFrameIndex(FI, MVT::i32);
-      InVals.push_back(DAG.getLoad(VA.getLocVT(), dl, Chain, FIN,
-                                   MachinePointerInfo::getFixedStack(FI),
-                                   false, false, false, 0));
+      SDValue GLN = DAG.getLoad(VA.getLocVT(), dl, Chain, FIN,
+                                MachinePointerInfo::getFixedStack(FI),
+                                false, false, false, 0);
+      if (Flags.isByVal() && Flags.getByValSize()){
+        // delay creating copy of byval argument & adding result to InVals
+        const ByValPair BVD = {GLN,Flags};
+        BVData.push_back(BVD);
+      } else {
+        InVals.push_back(GLN);
+      }
You could avoid duplicating this code by moving it after the if / else.
     }
   }



+  // 1b. CopyFromReg vararg registers
   if (isVarArg) {
     /* Argument registers */
     static const uint16_t ArgRegs[] = {
@@ -1134,7 +1163,6 @@ XCoreTargetLowering::LowerCCCArguments(SDValue Chain,
     unsigned FirstVAReg = CCInfo.getFirstUnallocated(ArgRegs,
                                                      array_lengthof(ArgRegs));
     if (FirstVAReg < array_lengthof(ArgRegs)) {
-      SmallVector<SDValue, 4> MemOps;
       int offset = 0;
       // Save remaining registers, storing higher register numbers at a higher
       // address
@@ -1150,14 +1178,12 @@ XCoreTargetLowering::LowerCCCArguments(SDValue Chain,
         unsigned VReg = RegInfo.createVirtualRegister(&XCore::GRRegsRegClass);
         RegInfo.addLiveIn(ArgRegs[i], VReg);
         SDValue Val = DAG.getCopyFromReg(Chain, dl, VReg, MVT::i32);
+        RegNode.push_back(Val.getValue(Val->getNumValues()-1));
LLVM Style is to have spaces around '-',
         // Move argument from virt reg -> stack
         SDValue Store = DAG.getStore(Val.getValue(1), dl, Val, FIN,
                                      MachinePointerInfo(), false, false, 0);
         MemOps.push_back(Store);
       }
-      if (!MemOps.empty())
-        Chain = DAG.getNode(ISD::TokenFactor, dl, MVT::Other,
-                            &MemOps[0], MemOps.size());
     } else {
       // This will point to the next argument passed via stack.
       XFI->setVarArgsFrameIndex(
@@ -1166,6 +1192,39 @@ XCoreTargetLowering::LowerCCCArguments(SDValue Chain,
     }
   }



+  // 2. chain CopyFromReg nodes into a TokenFactor
+  if (!RegNode.empty())
+    Chain = DAG.getNode(ISD::TokenFactor, dl, MVT::Other,
+                        &RegNode[0], RegNode.size());
+
+  // 3. Memcpy any 'byVal' args
+  if (!BVData.empty()) {
+    // aggregates passed "byVal" need to be copied by the callee
+    // and a pointer to the copy used by the callee instead of
+    // the original pointer (which is now in 'BVData.SDV')
Comments should be sentences with proper capitalisation and punctuation, see http://llvm.org/docs/CodingStandards.html#commenting


+    for (SmallVectorImpl<ByValPair>::const_iterator
+         BVDI = BVData.begin(), BVDE = BVData.end(); BVDI != BVDE; ++BVDI) {
+      unsigned Size = BVDI->Flags.getByValSize();
+      unsigned Align = BVDI->Flags.getByValAlign();
+      // Create a new object on the stack and copy the pointee into it.
+      int FI = MFI->CreateStackObject(Size, Align, false, false);
+      SDValue FIN = DAG.getFrameIndex(FI, MVT::i32);
+      InVals.push_back(FIN);
+      MemOps.push_back(DAG.getMemcpy(Chain, dl, FIN, BVDI->SDV,
+                                     DAG.getConstant(Size, MVT::i32),
+                                     Align, false, false,
+                                     MachinePointerInfo(),
+                                     MachinePointerInfo()));
+    }
+  }
+
+  // 4, chain all the mem ops nodes into a TokenFactor
+  if (!MemOps.empty()){
Should be a space between ')' and '{'
+    MemOps.push_back(Chain);
+    Chain = DAG.getNode(ISD::TokenFactor, dl, MVT::Other,
+                        &MemOps[0], MemOps.size());
+  }
+
   return Chain;
 }



diff --git a/test/CodeGen/XCore/byVal.ll b/test/CodeGen/XCore/byVal.ll
index 0000000..18af006
--- /dev/null
+++ b/test/CodeGen/XCore/byVal.ll
@@ -0,0 +1,57 @@
+; RUN: llc < %s -march=xcore | FileCheck %s
+
+; CHECK-LABEL: f0Test
+; CHECK: entsp 1
+; CHECK: bl f0
+; CHECK: retsp 1
+%struct.st0 = type { [0 x i32] }
+declare void @f0(%struct.st0*) nounwind
+define void @f0Test(%struct.st0* byval %s0) nounwind {
+entry:
+  call void @f0(%struct.st0* %s0) nounwind
+  ret void
+}
+
+; CHECK-LABEL: f1Test
+; CHECK: entsp 13
+; CHECK: stw r4, sp[12]
+; CHECK: stw r5, sp[11]
+; CHECK: mov r4, r0
+; CHECK: ldaw r5, sp[1]
+; CHECK: ldc r2, 40
+; CHECK: mov r0, r5
+; CHECK: bl memcpy
+; CHECK: mov r0, r5
+; CHECK: bl f1
+; CHECK: mov r0, r4
+; CHECK: ldw r5, sp[11]
+; CHECK: ldw r4, sp[12]
+; CHECK: retsp 13
+%struct.st1 = type { [10 x i32] }
+declare void @f1(%struct.st1*) nounwind
+define i32 @f1Test(i32 %i, %struct.st1* byval %s1) nounwind {
+entry:
+  call void @f1(%struct.st1* %s1) nounwind
+  ret i32 %i
+}
+
+; CHECK-LABEL: f2Test
+; CHECK: extsp 5
+; CHECK: stw lr, sp[1]
+; CHECK: stw r1, sp[3]
+; CHECK: stw r2, sp[4]
+; CHECK: stw r3, sp[5]
+; CHECK: ldw r0, r0[0]
+; CHECK: stw r0, sp[2]
+; CHECK: ldaw r0, sp[2]
+; CHECK: bl f2
+; CHECK: ldw lr, sp[1]
+; CHECK: ldaw sp, sp[5]
+; CHECK: retsp 0
+%struct.st2 = type { i32 }
+declare void @f2(%struct.st2*) nounwind
+define void @f2Test(%struct.st2* byval %s2, ...) nounwind {
+entry:
+  call void @f2(%struct.st2* %s2) nounwind
+  ret void
+}

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130725/2070ca65/attachment.html>


More information about the llvm-commits mailing list