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

[Anatoly Trosinenko via Phabricator via cfe-commits]

atrosinenko added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85917/new/

https://reviews.llvm.org/D85917

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Anton Korobeynikov via Phabricator via cfe-commits]

asl added a comment.

In D85917#4523503 , @arsenm wrote:

> Is there a reason this never landed?

Good question. @atrosinenko ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85917/new/

https://reviews.llvm.org/D85917

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added a comment.
Herald added a project: All.

Is there a reason this never landed?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85917/new/

https://reviews.llvm.org/D85917

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

LGTM




Comment at: clang/lib/CodeGen/TargetInfo.cpp:7523
+  return ABIArgInfo::getIndirectAliased(
+  getContext().getTypeAlignInChars(Ty), /*AddrSpace=*/0);
+

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85917/new/

https://reviews.llvm.org/D85917

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Anatoly Trosinenko via Phabricator via cfe-commits]

atrosinenko updated this revision to Diff 286038.
atrosinenko added a comment.

Update with implicit pointers being passed through.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85917/new/

https://reviews.llvm.org/D85917

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/msp430-struct-or-union-args.c

Index: clang/test/CodeGen/msp430-struct-or-union-args.c
===
--- /dev/null
+++ clang/test/CodeGen/msp430-struct-or-union-args.c
@@ -0,0 +1,123 @@
+// 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 
+#include 
+
+// 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();
+}
+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();
+}
+
+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);
+}
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -7511,11 +7511,18 @@
 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 

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

[Anatoly Trosinenko via Phabricator via cfe-commits]

atrosinenko updated this revision to Diff 285436.
atrosinenko added a comment.

Restore test formatting.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85917/new/

https://reviews.llvm.org/D85917

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/msp430-struct-or-union-args.c

Index: clang/test/CodeGen/msp430-struct-or-union-args.c
===
--- /dev/null
+++ clang/test/CodeGen/msp430-struct-or-union-args.c
@@ -0,0 +1,91 @@
+// REQUIRES: msp430-registered-target
+// RUN: %clang -target msp430 -fno-inline-functions -S -o- %s | FileCheck --check-prefixes=ASM %s
+// RUN: %clang -target msp430 -fno-inline-functions -S -emit-llvm -o- %s | FileCheck --check-prefixes=IR %s
+
+#include 
+#include 
+
+// 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();
+}
+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();
+}
+
+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);
+}
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -7511,11 +7511,18 @@
 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
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -4474,23 +4474,24 @@
 } else if (I->hasLValue()) {
   auto LV = I->getKnownLValue();
   auto AS = LV.getAddressSpace();
+  bool isByValOrRef =
+  ArgInfo.isIndirectAliased() || ArgInfo.getIndirectByVal();
 
-  if (!ArgInfo.getIndirectByVal() ||
+  if (!isByValOrRef ||
   

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

[Anatoly Trosinenko via Phabricator via cfe-commits]

atrosinenko created this revision.
atrosinenko added reviewers: asl, krisb, arsenm, rjmccall, jdoerfert.
Herald added a project: clang.
atrosinenko requested review of this revision.
Herald added a subscriber: wdng.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D85917

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/msp430-struct-or-union-args.c

Index: clang/test/CodeGen/msp430-struct-or-union-args.c
===
--- /dev/null
+++ clang/test/CodeGen/msp430-struct-or-union-args.c
@@ -0,0 +1,91 @@
+// REQUIRES: msp430-registered-target
+// RUN: %clang -target msp430 -fno-inline-functions -S -o- %s | FileCheck --check-prefixes=ASM %s
+// RUN: %clang -target msp430 -fno-inline-functions -S -emit-llvm -o- %s | FileCheck --check-prefixes=IR %s
+
+#include 
+#include 
+
+// 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();
+}
+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();
+}
+
+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);
+}
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -7511,11 +7511,18 @@
 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
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -4474,23