Fix ARM AAPCS regression caused by r211898

[olista01]

Summary

r211898 introduced a regression where a large struct, which would normally be passed ByVal, was causing padding to be inserted to prevent the backend from using some GPRs, in order to follow the AAPCS. However, the type of the argument was not being set correctly, so the backend cannot align 8-byte aligned struct types on the stack.

The fix is to not insert the padding arguments when the argument is being passed ByVal.

[rengolin]

I don't think there are enough changes here to warrant a bigger refactoring, but this mechanism has proven to be fragile, given the number of "this commit broke byval" post-commits.

I'm not against this particular patch, but I'd like to see a bit more work on making the code robust, not just adding special cases...

cheers,
--renato

lib/CodeGen/TargetInfo.cpp

4004

This is getting slightly out of hand...

test/CodeGen/arm-aapcs-vfp.c

150

and this relationship between the front-end and the back-end is getting thinner...

[olista01]

We could simply disable the use of byval for ARM, as it is not required for ABI correctness. Two of my 5 most recent ABI-related commits have been directly related to byval arguments. Do you know what the purpose of byval is, and if there would be any downside to not using it? Does it allow for some optimisation at the IR level? Even if it does, it is probably not a very significant gain as it is only used for very large types passed by value, and the implementation in the ARM backend generates unnecessary stores to the stack in a lot of cases.

[rengolin]

Hi Oliver,

ByVal is normally a big deal for performance when structures are passed as arguments, and I don't think we should disable it like that. I don't have benchmark numbers to tell if it really is worth keep it, maybe you can disable it and run some benchmarks.

All in all, this commit looks ok and if no one is against it, feel free to commit. We can leave the ByVal discussion for later.

cheers,
--renato

[olista01]

Committed revision 213359.

I'll have a look and see if we have any benchmarks that this might affect, but I have a feeling that passing 64 byte+ structs by value is not a common thing to do, especially in performance-sensitive code.

lib/CodeGen/TargetInfo.cpp

for (auto &I : FI.arguments()) {
     I.info = classifyArgumentType(I.type, FI.isVariadic(), IsCPRC);
 
     // If we have allocated some arguments onto the stack (due to running
     // out of VFP registers), we cannot split an argument between GPRs and
     // the stack. If this situation occurs, we add padding to prevent the
     // GPRs from being used. In this situation, the current argument could
     // only be allocated by rule C.8, so rule C.6 would mark these GPRs as
     // unusable anyway.
+    // We do not have to do this if the argument is being passed ByVal, as the
+    // backend can handle that situation correctly.
     const bool StackUsed = PreAllocationGPRs > NumGPRs || PreAllocationVFPs > NumVFPs;
-    if (!IsCPRC && PreAllocationGPRs < NumGPRs && AllocatedGPRs > NumGPRs && StackUsed) {
+    const bool IsByVal = I.info.isIndirect() && I.info.getIndirectByVal();
+    if (!IsCPRC && PreAllocationGPRs < NumGPRs && AllocatedGPRs > NumGPRs &&
+        StackUsed && !IsByVal) {
       llvm::Type *PaddingTy = llvm::ArrayType::get(
           llvm::Type::getInt32Ty(getVMContext()), NumGPRs - PreAllocationGPRs);
       if (I.info.canHaveCoerceToType()) {
         I.info = ABIArgInfo::getDirect(I.info.getCoerceToType() /* type */, 0 /* offset */,
                                        PaddingTy);
       } else {
         I.info = ABIArgInfo::getDirect(nullptr /* type */, 0 /* offset */,
                                        PaddingTy);

test/CodeGen/arm-aapcs-vfp.c

 // 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) {}
+
+// Here, parameter k would need padding to prevent it from being split, but it
+// is passed ByVal (due to being > 64 bytes), so the backend handles this instead.
+void test_vfp_stack_gpr_split_6(double a, double b, double c, double d, double e, double f, double g, double h, double i, int j, struct_seventeen_ints k) {}
+// CHECK: define arm_aapcs_vfpcc void @test_vfp_stack_gpr_split_6(double %a, double %b, double %c, double %d, double %e, double %f, double %g, double %h, double %i, i32 %j, %struct.struct_seventeen_ints* byval align 4 %k)