[ARM] Fix bug 39982 - pcs("aapcs-vfp") is not consistent

[carwil]

Summary

See: https://bugs.llvm.org/show_bug.cgi?id=39982

When considering how to classify homogeneous aggregates as return/argument types, the ABI of the function (specified by attribute pcs) wasn't being taken into account. This resulted in some weird and unexpected hybrid assembly when compiling with softfp.

[efriedma]

It looks like every call to getABIKind() is doing the wrong thing; I'm worried that's going to lead to some sort of subtle miscompile without some sort of centralized fix to compute the correct calling convention for every place that checks it. Not sure how to write a testcase off the top of my head... have you tried homogeneous aggregates with more than two elements?

clang/test/CodeGenCXX/arm-pcs.cpp

4 ↗

(On Diff #189714)

This test needs "// REQUIRES: arm-registered-target" so it's automatically disabled if the ARM target isn't available.

[carwil]

In D59094#1421893, @efriedma wrote:

Not sure how to write a testcase off the top of my head... have you tried homogeneous aggregates with more than two elements?

Good catch! Seems like if we surpass "isHomogeneousAggregateSmallEnough" (4), it's no longer classified as an aggregate and so doesn't benefit from this fix. I'll see if I can work out a way to catch that as well.

It looks like every call to getABIKind() is doing the wrong thing; I'm worried that's going to lead to some sort of subtle miscompile without some sort of centralized fix to compute the correct calling convention for every place that checks it.

Agreed. It would be much better if this determined the ABI on context. That said, it doesn't look like that's going to be trivial to achieve...

[carwil]

I got bit a confused earlier. That does actually seem like correct behaviour (once we're no longer able to treat the struct as a homogeneous aggregate).
I've tightened up the conditional a bit as it wouldn't have previously accounted for the case where you had mfloat-abi=hard with an AAPCS (non VFP) attribute. Although in this instance, the backend does seem to be correcting/ignoring that, it's still best we don't blindly rely on such behaviour.

I've also re-structured the conditional and added some comments so it's (hopefully) a little bit clearer what's going on.

I believe this fixes the bug at hand. There's some other calls to getABIKind referencing AAPCS16_VFP, but that doesn't have a direct translation to an LLVM/FI CallingConvention (that I can see). Seems there is a lot of different ways to check various ABI/CC's and none of them seem able to give you the full picture at any point.

[efriedma]

Looking more carefully, I guess none of the other places actually distinguish between ARMABIInfo::AAPCS_VFP and ARMABIInfo::AAPCS, so I guess the current change is sufficient given the calling-convention attributes that are likely to be used in practice.

Please pull the IsEffectivelyAAPCS_VFP out into a separate helper function on ARMABIInfo.

[carwil]

Done. I've added an extra parameter over what you might expect as classifyArgumentTypes doesn't seem to consider AAPCS16, whereas classifyReturnTypes does.
I've also renamed the variable to make the distinction between it and the function clearer.

[efriedma]

LGTM

clang/lib/CodeGen/TargetInfo.cpp

6023 ↗

(On Diff #190798)

Variable name should be capitalized.

lib/CodeGen/TargetInfo.cpp

bool isEABIHF() const {
     default:
       return false;
     }
   }
 
   ABIKind getABIKind() const { return Kind; }
 
 private:
-  ABIArgInfo classifyReturnType(QualType RetTy, bool isVariadic) const;
-  ABIArgInfo classifyArgumentType(QualType RetTy, bool isVariadic) const;
+  ABIArgInfo classifyReturnType(QualType RetTy, bool isVariadic,
+                                unsigned functionCallConv) const;
+  ABIArgInfo classifyArgumentType(QualType RetTy, bool isVariadic,
+                                  unsigned functionCallConv) const;
   ABIArgInfo classifyHomogeneousAggregate(QualType Ty, const Type *Base,
                                           uint64_t Members) const;
   ABIArgInfo coerceIllegalVector(QualType Ty) const;
   bool isIllegalVectorType(QualType Ty) const;
 
   bool isHomogeneousAggregateBaseType(QualType Ty) const override;
   bool isHomogeneousAggregateSmallEnough(const Type *Ty,
                                          uint64_t Members) const override;
 
+  bool isEffectivelyAAPCS_VFP(unsigned callConvention, bool acceptHalf) const;
+
   void computeInfo(CGFunctionInfo &FI) const override;
 
   Address EmitVAArg(CodeGenFunction &CGF, Address VAListAddr,
                     QualType Ty) const override;
 
   llvm::CallingConv::ID getLLVMDefaultCC() const;
   llvm::CallingConv::ID getABIDefaultCC() const;
   void setCCs();
void WindowsARMTargetCodeGenInfo::setTargetAttributes(
   if (GV->isDeclaration())
     return;
   addStackProbeTargetAttributes(D, GV, CGM);
 }
 }
 
 void ARMABIInfo::computeInfo(CGFunctionInfo &FI) const {
   if (!::classifyReturnType(getCXXABI(), FI, *this))
-    FI.getReturnInfo() =
-        classifyReturnType(FI.getReturnType(), FI.isVariadic());
+    FI.getReturnInfo() = classifyReturnType(FI.getReturnType(), FI.isVariadic(),
+                                            FI.getCallingConvention());
 
   for (auto &I : FI.arguments())
-    I.info = classifyArgumentType(I.type, FI.isVariadic());
+    I.info = classifyArgumentType(I.type, FI.isVariadic(),
+                                  FI.getCallingConvention());
+
 
   // Always honor user-specified calling convention.
   if (FI.getCallingConvention() != llvm::CallingConv::C)
     return;
 
   llvm::CallingConv::ID cc = getRuntimeCC();
   if (cc != llvm::CallingConv::C)
     FI.setEffectiveCallingConvention(cc);
if (!getTarget().hasLegalHalfType() &&
           llvm::Type::getInt32Ty(getVMContext()), Size / 32);
       llvm::Type *Ty = llvm::ArrayType::get(NewVecTy, Members);
       return ABIArgInfo::getDirect(Ty, 0, nullptr, false);
     }
   }
   return ABIArgInfo::getDirect(nullptr, 0, nullptr, false);
 }
 
-ABIArgInfo ARMABIInfo::classifyArgumentType(QualType Ty,
-                                            bool isVariadic) const {
+ABIArgInfo ARMABIInfo::classifyArgumentType(QualType Ty, bool isVariadic,
+                                            unsigned functionCallConv) const {
   // 6.1.2.1 The following argument types are VFP CPRCs:
   //   A single-precision floating-point type (including promoted
   //   half-precision types); A double-precision floating-point type;
   //   A 64-bit or 128-bit containerized vector type; Homogeneous Aggregate
   //   with a Base Type of a single- or double-precision floating-point type,
   //   64-bit containerized vectors or 128-bit containerized vectors with one
   //   to four Elements.
-  bool IsEffectivelyAAPCS_VFP = getABIKind() == AAPCS_VFP && !isVariadic;
+  // Variadic functions should always marshal to the base standard.
+  bool IsAAPCS_VFP =
+      !isVariadic && isEffectivelyAAPCS_VFP(functionCallConv, /* AAPCS16 */ false);
 
   Ty = useFirstFieldIfTransparentUnion(Ty);
 
   // Handle illegal vector types here.
   if (isIllegalVectorType(Ty))
     return coerceIllegalVector(Ty);
 
   // _Float16 and __fp16 get passed as if it were an int or float, but with
   // the top 16 bits unspecified. This is not done for OpenCL as it handles the
   // half type natively, and does not need to interwork with AAPCS code.
   if ((Ty->isFloat16Type() || Ty->isHalfType()) &&
       !getContext().getLangOpts().NativeHalfArgsAndReturns) {
-    llvm::Type *ResType = IsEffectivelyAAPCS_VFP ?
+    llvm::Type *ResType = IsAAPCS_VFP ?
       llvm::Type::getFloatTy(getVMContext()) :
       llvm::Type::getInt32Ty(getVMContext());
     return ABIArgInfo::getDirect(ResType);
   }
 
   if (!isAggregateTypeForABI(Ty)) {
     // Treat an enum type as its underlying type.
     if (const EnumType *EnumTy = Ty->getAs<EnumType>()) {
       Ty = EnumTy->getDecl()->getIntegerType();
     }
 
     return (Ty->isPromotableIntegerType() ? ABIArgInfo::getExtend(Ty)
                                           : ABIArgInfo::getDirect());
   }
 
   if (CGCXXABI::RecordArgABI RAA = getRecordArgABI(Ty, getCXXABI())) {
     return getNaturalAlignIndirect(Ty, RAA == CGCXXABI::RAA_DirectInMemory);
   }
 
   // Ignore empty records.
   if (isEmptyRecord(getContext(), Ty, true))
     return ABIArgInfo::getIgnore();
 
-  if (IsEffectivelyAAPCS_VFP) {
+  if (IsAAPCS_VFP) {
     // Homogeneous Aggregates need to be expanded when we can fit the aggregate
     // into VFP registers.
     const Type *Base = nullptr;
     uint64_t Members = 0;
     if (isHomogeneousAggregate(Ty, Base, Members))
       return classifyHomogeneousAggregate(Ty, Base, Members);
   } else if (getABIKind() == ARMABIInfo::AAPCS16_VFP) {
     // WatchOS does have homogeneous aggregates. Note that we intentionally use
if (!RD->isUnion()) {
 
       HadField = true;
     }
   }
 
   return true;
 }
 
-ABIArgInfo ARMABIInfo::classifyReturnType(QualType RetTy,
-                                          bool isVariadic) const {
-  bool IsEffectivelyAAPCS_VFP =
-      (getABIKind() == AAPCS_VFP || getABIKind() == AAPCS16_VFP) && !isVariadic;
+ABIArgInfo ARMABIInfo::classifyReturnType(QualType RetTy, bool isVariadic,
+                                          unsigned functionCallConv) const {
+
+  // Variadic functions should always marshal to the base standard.
+  bool IsAAPCS_VFP =
+      !isVariadic && isEffectivelyAAPCS_VFP(functionCallConv, /* AAPCS16 */ true);
 
   if (RetTy->isVoidType())
     return ABIArgInfo::getIgnore();
 
   if (const VectorType *VT = RetTy->getAs<VectorType>()) {
     // Large vector types should be returned via memory.
     if (getContext().getTypeSize(RetTy) > 128)
       return getNaturalAlignIndirect(RetTy);
     // FP16 vectors should be converted to integer vectors
     if (!getTarget().hasLegalHalfType() &&
         (VT->getElementType()->isFloat16Type() ||
          VT->getElementType()->isHalfType()))
       return coerceIllegalVector(RetTy);
   }
 
   // _Float16 and __fp16 get returned as if it were an int or float, but with
   // the top 16 bits unspecified. This is not done for OpenCL as it handles the
   // half type natively, and does not need to interwork with AAPCS code.
   if ((RetTy->isFloat16Type() || RetTy->isHalfType()) &&
       !getContext().getLangOpts().NativeHalfArgsAndReturns) {
-    llvm::Type *ResType = IsEffectivelyAAPCS_VFP ?
+    llvm::Type *ResType = IsAAPCS_VFP ?
       llvm::Type::getFloatTy(getVMContext()) :
       llvm::Type::getInt32Ty(getVMContext());
     return ABIArgInfo::getDirect(ResType);
   }
 
   if (!isAggregateTypeForABI(RetTy)) {
     // Treat an enum type as its underlying type.
     if (const EnumType *EnumTy = RetTy->getAs<EnumType>())
ABIArgInfo ARMABIInfo::classifyReturnType(QualType RetTy, bool isVariadic,
   }
 
   // Otherwise this is an AAPCS variant.
 
   if (isEmptyRecord(getContext(), RetTy, true))
     return ABIArgInfo::getIgnore();
 
   // Check for homogeneous aggregates with AAPCS-VFP.
-  if (IsEffectivelyAAPCS_VFP) {
+  if (IsAAPCS_VFP) {
     const Type *Base = nullptr;
     uint64_t Members = 0;
     if (isHomogeneousAggregate(RetTy, Base, Members))
       return classifyHomogeneousAggregate(RetTy, Base, Members);
   }
 
   // Aggregates <= 4 bytes are returned in r0; other aggregates
   // are returned indirectly.
bool ARMABIInfo::isHomogeneousAggregateBaseType(QualType Ty) const {
   return false;
 }
 
 bool ARMABIInfo::isHomogeneousAggregateSmallEnough(const Type *Base,
                                                    uint64_t Members) const {
   return Members <= 4;
 }
 
+bool ARMABIInfo::isEffectivelyAAPCS_VFP(unsigned callConvention,
+                                        bool acceptHalf) const {
+  // Give precedence to user-specified calling conventions.
+  if (callConvention != llvm::CallingConv::C)
+    return (callConvention == llvm::CallingConv::ARM_AAPCS_VFP);
+  else
+    return (getABIKind() == AAPCS_VFP) ||
+           (acceptHalf && (getABIKind() == AAPCS16_VFP));
+}
+
 Address ARMABIInfo::EmitVAArg(CodeGenFunction &CGF, Address VAListAddr,
                               QualType Ty) const {
   CharUnits SlotSize = CharUnits::fromQuantity(4);
 
   // Empty records are ignored for parameter passing purposes.
   if (isEmptyRecord(getContext(), Ty, true)) {
     Address Addr(CGF.Builder.CreateLoad(VAListAddr), SlotSize);
     Addr = CGF.Builder.CreateElementBitCast(Addr, CGF.ConvertTypeForMem(Ty));

test/CodeGenCXX/arm-pcs.cpp

+// Covers a bug fix for ABI selection with homogenous aggregates:
+//  See: https://bugs.llvm.org/show_bug.cgi?id=39982
+
+// REQUIRES: arm-registered-target
+// RUN: %clang -mfloat-abi=hard --target=armv7-unknown-linux-gnueabi -O3 -S -o - %s | FileCheck %s -check-prefixes=HARD,CHECK
+// RUN: %clang -mfloat-abi=softfp --target=armv7-unknown-linux-gnueabi -O3 -S -o - %s | FileCheck %s -check-prefixes=SOFTFP,CHECK
+// RUN: %clang -mfloat-abi=soft --target=armv7-unknown-linux-gnueabi -O3 -S -o - %s | FileCheck %s -check-prefixes=SOFT,CHECK
+
+struct S {
+  float f;
+  float d;
+  float c;
+  float t;
+};
+
+// Variadic functions should always marshal for the base standard.
+// See section 5.5 (Parameter Passing) of the AAPCS.
+float __attribute__((pcs("aapcs-vfp"))) variadic(S s, ...) {
+  // CHECK-NOT: vmov s{{[0-9]+}}, s{{[0-9]+}}
+  // CHECK: mov r{{[0-9]+}}, r{{[0-9]+}}
+  return s.d;
+}
+
+float no_attribute(S s) {
+  // SOFT: mov r{{[0-9]+}}, r{{[0-9]+}}
+  // SOFTFP: mov r{{[0-9]+}}, r{{[0-9]+}}
+  // HARD: vmov.f32 s{{[0-9]+}}, s{{[0-9]+}}
+  return s.d;
+}
+
+float __attribute__((pcs("aapcs-vfp"))) baz(float x, float y) {
+  // CHECK-NOT: mov s{{[0-9]+}}, r{{[0-9]+}}
+  // SOFT: mov r{{[0-9]+}}, r{{[0-9]+}}
+  // SOFTFP: vmov.f32 s{{[0-9]+}}, s{{[0-9]+}}
+  // HARD: vmov.f32 s{{[0-9]+}}, s{{[0-9]+}}
+  return y;
+}
+
+float __attribute__((pcs("aapcs-vfp"))) foo(S s) {
+  // CHECK-NOT: mov s{{[0-9]+}}, r{{[0-9]+}}
+  // SOFT: mov r{{[0-9]+}}, r{{[0-9]+}}
+  // SOFTFP: vmov.f32 s{{[0-9]+}}, s{{[0-9]+}}
+  // HARD: vmov.f32 s{{[0-9]+}}, s{{[0-9]+}}
+  return s.d;
+}
+
+float __attribute__((pcs("aapcs"))) bar(S s) {
+  // CHECK-NOT: vmov.f32 s{{[0-9]+}}, s{{[0-9]+}}
+  // CHECK: mov r{{[0-9]+}}, r{{[0-9]+}}
+  return s.d;
+}