[MSP430] Fix passing C structs and unions as function arguments

[atrosinenko]

Summary

Pass structures, classes and unions by reference on MSP430, according to Section 3.5 of MSP430 EABI. The callee is responsible for leaving the underlying data intact.

[atrosinenko]

Restore test formatting.

[atrosinenko]

Update with implicit pointers being passed through.

[rjmccall]

LGTM

clang/lib/CodeGen/TargetInfo.cpp

7523

Oh wow, the ABI really is indirect aliased:

To pass a structure or union by reference, the caller places its address
in the appropriate location: either in a register or on the stack, according
to its position in the argument list. To preserve pass-by-value semantics
(required for C and C++), the callee may need to make its own copy of the
pointed-to object. In some cases, the callee need not make a copy, such
as if the callee is a leaf and it does not modify the pointed-to object.

I didn't realize any actual ABIs did this.

[arsenm]

Is there a reason this never landed?

[asl]

In D85917#4523503, @arsenm wrote:

Is there a reason this never landed?

Good question. @atrosinenko ?

[atrosinenko]

Sorry for a hung patch. After getting the approval I had doubts that I interpret the corner cases of LLVM IR correctly. Thus, I postponed the patch these days to not subtly break the stable targets while fixing one issue on MSP430.

I can re-evaluate this patch if it is still relevant.

clang/lib/CodeGen/CGCall.cpp

case ABIArgInfo::IndirectAliased: {
 
         if (Addr.getAlignment() < Align &&
             llvm::getOrEnforceKnownAlignment(V, Align.getAsAlign(), *TD) <
                 Align.getAsAlign()) {
           NeedCopy = true;
         } else if (I->hasLValue()) {
           auto LV = I->getKnownLValue();
           auto AS = LV.getAddressSpace();
+          bool isByValOrRef =
+              ArgInfo.isIndirectAliased() || ArgInfo.getIndirectByVal();
 
-          if (!ArgInfo.getIndirectByVal() ||
+          if (!isByValOrRef ||
               (LV.getAlignment() < getContext().getTypeAlignInChars(I->Ty))) {
             NeedCopy = true;
           }
           if (!getLangOpts().OpenCL) {
-            if ((ArgInfo.getIndirectByVal() &&
-                (AS != LangAS::Default &&
+            if ((isByValOrRef && (AS != LangAS::Default &&
                                   AS != CGM.getASTAllocaAddressSpace()))) {
               NeedCopy = true;
             }
           }
           // For OpenCL even if RV is located in default or alloca address space
           // we don't want to perform address space cast for it.
-          else if ((ArgInfo.getIndirectByVal() &&
-                    Addr.getType()->getAddressSpace() != IRFuncTy->
-                      getParamType(FirstIRArg)->getPointerAddressSpace())) {
+          else if ((isByValOrRef && Addr.getType()->getAddressSpace() !=
+                                        IRFuncTy->getParamType(FirstIRArg)
+                                            ->getPointerAddressSpace())) {
             NeedCopy = true;
           }
         }
 
         if (NeedCopy) {
           // Create an aligned temporary, and copy to it.
           Address AI = CreateMemTempWithoutCast(
               I->Ty, ArgInfo.getIndirectAlign(), "byval-temp");

clang/lib/CodeGen/TargetInfo.cpp

public:
 
   ABIArgInfo classifyReturnType(QualType RetTy) const {
     if (RetTy->isAnyComplexType())
       return complexArgInfo();
 
     return DefaultABIInfo::classifyReturnType(RetTy);
   }
 
-  ABIArgInfo classifyArgumentType(QualType RetTy) const {
-    if (RetTy->isAnyComplexType())
+  ABIArgInfo classifyArgumentType(QualType Ty) const {
+    if (Ty->isAnyComplexType())
       return complexArgInfo();
 
-    return DefaultABIInfo::classifyArgumentType(RetTy);
+    // According to Section 3.5 of MSP430 EABI, structs, classes and unions
+    // are passed by reference. The callee is responsible for leaving them
+    // intact.
+    if (Ty->isStructureOrClassType() || Ty->isUnionType())
+      return ABIArgInfo::getIndirectAliased(
+          getContext().getTypeAlignInChars(Ty), /*AddrSpace=*/0);
+
+    return DefaultABIInfo::classifyArgumentType(Ty);
   }
 
   // Just copy the original implementations because
   // DefaultABIInfo::classify{Return,Argument}Type() are not virtual
   void computeInfo(CGFunctionInfo &FI) const override {
     if (!getCXXABI().classifyReturnType(FI))
       FI.getReturnInfo() = classifyReturnType(FI.getReturnType());
     for (auto &I : FI.arguments())

clang/test/CodeGen/msp430-struct-or-union-args.c

+// REQUIRES: msp430-registered-target
+// Optimized to check that some of memcpy intrinsic invocations are optimized out.
+// RUN: %clang -target msp430 -fno-inline-functions -S -Os -o- %s | FileCheck --check-prefixes=ASM %s
+// Do not use any optimization to not clutter the output with deduced LLVM IR attributes.
+// RUN: %clang -target msp430 -fno-inline-functions -S -emit-llvm -o- %s | FileCheck --check-prefixes=IR %s
+
+#include <limits.h>
+#include <stdint.h>
+
+// According to Section 3.5 of MSP430 EABI, structures and unions are passed
+// by reference, even if an equally-sized integer argument could be passed
+// in registers.
+
+struct S {
+  uint16_t a;
+};
+union U {
+  uint16_t a;
+};
+
+// IR: %struct.S = type { i16 }
+// IR: %union.U = type { i16 }
+
+_Static_assert(sizeof(struct S) * CHAR_BIT == 16, "Unexpected size");
+_Static_assert(sizeof(union U) * CHAR_BIT == 16, "Unexpected size");
+
+extern struct S s;
+extern union U u;
+
+// Cannot know for sure whether they change the argument
+extern void leaf_s(struct S *x);
+extern void leaf_u(union U *);
+
+// Callee is responsible for leaving the byref argument intact
+void middle_s(struct S x) {
+// IR: define {{(dso_local )?}}void @middle_s(%struct.S* byref(%struct.S) align 2 [[S_ARG:%.+]])
+// IR: [[S_COPY:%.+]] = alloca %struct.S, align 2
+// IR: [[S_CASTED_COPY:%.+]] = bitcast %struct.S* [[S_COPY]] to i8*
+// IR: [[S_CASTED_ARG:%.+]] = bitcast %struct.S* [[S_ARG]] to i8*
+// IR: call void @llvm.memcpy.p0i8.p0i8.i16(i8* align 2 [[S_CASTED_COPY]], i8* align 2 [[S_CASTED_ARG]], i16 2, i1 false)
+// IR: call void @leaf_s(%struct.S* [[S_COPY]])
+// IR: ret void
+
+// ASM:      middle_s:
+// ASM:      sub #2, r1
+// ... here memcpy() occurs ...
+// ASM:      mov r1, r12
+// ASM-NEXT: call #leaf_s
+// ASM-NEXT: add #2, r1
+// ASM-NEXT: ret
+  leaf_s(&x);
+}
+void middle_u(union U x) {
+// IR: define {{(dso_local )?}}void @middle_u(%union.U* byref(%union.U) align 2 [[U_ARG:%.+]])
+// IR: [[U_COPY:%.+]] = alloca %union.U, align 2
+// IR: [[U_CASTED_COPY:%.+]] = bitcast %union.U* [[U_COPY]] to i8*
+// IR: [[U_CASTED_ARG:%.+]] = bitcast %union.U* [[U_ARG]] to i8*
+// IR: call void @llvm.memcpy.p0i8.p0i8.i16(i8* align 2 [[U_CASTED_COPY]], i8* align 2 [[U_CASTED_ARG]], i16 2, i1 false)
+// IR: call void @leaf_u(%union.U* [[U_COPY]])
+// IR: ret void
+
+// ASM:      middle_u:
+// ASM:      sub #2, r1
+// ... here memcpy() occurs ...
+// ASM:      mov r1, r12
+// ASM-NEXT: call #leaf_u
+// ASM-NEXT: add #2, r1
+// ASM-NEXT: ret
+  leaf_u(&x);
+}
+
+void caller_s(void) {
+// IR: define {{(dso_local )?}}void @caller_s()
+// IR: call void @middle_s(%struct.S* byref(%struct.S) align 2 @s)
+// IR: ret void
+
+// ASM:      caller_s:
+// ASM:      mov #s, r12
+// ASM-NEXT: call #middle_s
+// ASM-NEXT: ret
+  middle_s(s);
+}
+void caller_u(void) {
+// IR: define dso_local void @caller_u()
+// IR: call void @middle_u(%union.U* byref(%union.U) align 2 @u)
+// IR: ret void
+
+// ASM:      caller_u:
+// ASM:      mov #u, r12
+// ASM-NEXT: call #middle_u
+// ASM-NEXT: ret
+  middle_u(u);
+}
+
+// No need to create a temporary copy of the struct/union-typed argument
+// if it is just passed to other function as-is.
+// TODO For now, it works when size of structure is more than 8 bytes,
+// otherwise the memcpy intrinsic will be replaced by InstCombiner.
+
+struct LL {
+  long long a[2];
+};
+
+extern struct LL ll;
+
+extern void leaf(struct LL x);
+
+void middle(struct LL x) {
+// ASM:      middle:
+// No stack-allocated objects:
+// ASM-NOT:  r1
+// ASM: call #leaf
+// ASM-NEXT: ret
+  leaf(x);
+}
+
+void caller(void) {
+// ASM:      caller:
+// ASM:      mov #ll, r12
+// ASM-NEXT: call #middle
+// ASM-NEXT: ret
+  middle(ll);
+}