Fix AAPCS non-compliance caused by very large structs

[olista01]

Summary

This is a fix to the code in clang which inserts padding arguments to ensure that the ARM backend can emit AAPCS-VFP compliant code. This code needs to track the number of registers which have been allocated in order to do this. When passing a very large struct (>64 bytes) by value, clang emits IR which takes a pointer to the struct, but the backend converts this back to passing the struct in registers and on the stack. The bug was that this was being considered by clang to only use one register, meaning that there were situations in which padding arguments were incorrectly emitted by clang.

[rengolin]

lib/CodeGen/TargetInfo.cpp

4206

Couldn't this be:

NumRegs = (getContext().getTypeSize(Ty) + 63) / 32;
markAllocatedGPRs(2, NumRegs);

and avoid the extra multiplication?

[olista01]

Inline comment

lib/CodeGen/TargetInfo.cpp

4206

The division by 64 is part of rounding up to the nearest multiple of 64 [0], and this simplification would give different results if the size is, for example, 64 bits. That said, this code is only used when the size is greater than 64 bytes, meaning it will always use all available GPRs, so I only left the calculations in to make the intention clear. I wouldn't be averse to replacing both arms of the "if" with markAllocatedGPRs(1, 4) and a comment, if you would prefer that?

[0] This is because all types in the AAPCS have a size which is a multiple of their alignment.

[rengolin]

lib/CodeGen/TargetInfo.cpp

4206

Of course, silly me.

I think making it 4 and explaining why would make more sense.

[olista01]

Simplify, with comment explaining why this can be done.

This patch also fixes the (textual) alignment of the NumRequired parameter in the definition of ARMABIInfo::markAllocatedGPRs, which is present in the patch I uploaded but not showing up in Phab. This may be because it is a whitespace-only change?

[rengolin]

LGTM.

Thanks!
--renato

[olista01]

Thanks, Committed revision 211898.

lib/CodeGen/TargetInfo.cpp

for (unsigned I = 0; I < 16; I++)
     VFPRegs[I] = 1;
   AllocatedVFPs = 17; // We do not have enough VFP registers.
 }
 
 /// Update AllocatedGPRs to record the number of general purpose registers
 /// which have been allocated. It is valid for AllocatedGPRs to go above 4,
 /// this represents arguments being stored on the stack.
 void ARMABIInfo::markAllocatedGPRs(unsigned Alignment,
-                                          unsigned NumRequired) const {
   assert((Alignment == 1 || Alignment == 2) && "Alignment must be 4 or 8 bytes");
 
   if (Alignment == 2 && AllocatedGPRs & 0x1)
     AllocatedGPRs += 1;
 
   AllocatedGPRs += NumRequired;
 }
 
ABIArgInfo ARMABIInfo::classifyArgumentType(QualType Ty, bool isVariadic,
   // most 8-byte. We realign the indirect argument if type alignment is bigger
   // than ABI alignment.
   uint64_t ABIAlign = 4;
   uint64_t TyAlign = getContext().getTypeAlign(Ty) / 8;
   if (getABIKind() == ARMABIInfo::AAPCS_VFP ||
       getABIKind() == ARMABIInfo::AAPCS)
     ABIAlign = std::min(std::max(TyAlign, (uint64_t)4), (uint64_t)8);
   if (getContext().getTypeSizeInChars(Ty) > CharUnits::fromQuantity(64)) {
-    // Update Allocated GPRs
-    markAllocatedGPRs(1, 1);
+    // Update Allocated GPRs. Since this is only used when the size of the
+    // argument is greater than 64 bytes, this will always use up any available
+    // registers (of which there are 4). We also don't care about getting the
+    // alignment right, because general-purpose registers cannot be back-filled.
+    markAllocatedGPRs(1, 4);
     return ABIArgInfo::getIndirect(TyAlign, /*ByVal=*/true,
            /*Realign=*/TyAlign > ABIAlign);
   }
 
   // Otherwise, pass by coercing to a structure of the appropriate size.
   llvm::Type* ElemTy;
   unsigned SizeRegs;
   // FIXME: Try to match the types of the arguments more accurately where
   // we can.
   if (getContext().getTypeAlign(Ty) <= 32) {

test/CodeGen/arm-aapcs-vfp.c

 typedef struct { int a; int b:4; int c; } struct_int_bitfield_int;
 // CHECK: define arm_aapcs_vfpcc void @test_test_vfp_stack_gpr_split_bitfield(double %a, double %b, double %c, double %d, double %e, double %f, double %g, double %h, double %i, i32 %j, i32 %k, [2 x i32], { [3 x i32] } %l.coerce)
 void test_test_vfp_stack_gpr_split_bitfield(double a, double b, double c, double d, double e, double f, double g, double h, double i, int j, int k, struct_int_bitfield_int l) {}
 
 // Note: this struct requires internal padding
 typedef struct { int x; long long y; } struct_int_long_long;
 // CHECK: define arm_aapcs_vfpcc void @test_vfp_stack_gpr_split_4(double %a, double %b, double %c, double %d, double %e, double %f, double %g, double %h, double %i, i32 %j, [3 x i32], { [2 x i64] } %k.coerce)
 void test_vfp_stack_gpr_split_4(double a, double b, double c, double d, double e, double f, double g, double h, double i, int j, struct_int_long_long k) {}
+
+// This very large struct (passed byval) uses up the GPRs, so no padding is needed
+typedef struct { int x[17]; } struct_seventeen_ints;
+typedef struct { int x[4]; } struct_four_ints;
+// CHECK: define arm_aapcs_vfpcc void @test_vfp_stack_gpr_split_5(%struct.struct_seventeen_ints* byval align 4 %a, double %b, double %c, double %d, double %e, double %f, double %g, double %h, double %i, double %j, { [4 x i32] } %k.coerce)
+void test_vfp_stack_gpr_split_5(struct_seventeen_ints a, double b, double c, double d, double e, double f, double g, double h, double i, double j, struct_four_ints k) {}