[clang][aarch64] Precondition isHomogeneousAggregate on isCXX14Aggregate

[DavidTruby]

Summary

MSVC on WoA64 includes isCXX14Aggregate in its definition. This is de-facto
specification on that platform, so match msvc's behaviour.

Fixes: https://bugs.llvm.org/show_bug.cgi?id=47611

Co-authored-by: Peter Waller <[e-mail]>

[DavidTruby]

Added extra tests for additional conditions and IR -> assembly tests.

[DavidTruby]

Add additional callee-side tests

[rnk]

Thanks for the fix. At first, misunderstood, I expected that an aggregate containing non-aggregates should be returned indirectly, and that the fix would be in the C++ ABI codepath. However, I see that is not the case. An aggregate may contain non-aggregates, and MSVC will in fact return such a type directly in X0/X1.

clang/lib/CodeGen/CGCXXABI.cpp

320–321

I realize now that the name I chose (ixCXX14Aggregate) isn't very good because this routine also checks for trivial copy assignment and trivial destruction.

clang/lib/CodeGen/TargetInfo.cpp

5137–5142

This is target-independent code used on Mac, Linux, PPC64, etc. Please find a way to abstract over the triple check so that this is easier to read for the non-MSVC maintainer. Consider using a virtual method as is done below for isHomogeneousAggregateSmallEnough and is...BaseType.

5158–5160

This check seems suspect. I'd suggest constructing a test case involving zero width bitfields to see if we are compatible with MSVC.

clang/test/CodeGenCXX/microsoft-abi-sret-and-byval.cpp

472

Since the code change here affects the HVA classification codepath, I suggest you test this in clang/test/CodeGenCXX/homogeneous-aggregates.cpp.

They already have a number of corner case tests for this logic that we probably need to port to port anyway. Add a new RUN line, and new checks.

493

This comment doesn't seem accurate to me, it's returned indirectly in memory, right? The test for it below uses sret. In other words, this class never hits the HVA codepath because the C++ ABI marks it indirect.

502

This is a surprising finding: MSVC will return an aggregate which contains non-aggregates directly in registers. So in this example we get an X0/X1 return:
https://gcc.godbolt.org/z/xcjh4M

509–510

nit: I think the test case is simpler if you receive the pointer as a parameter and then forward it on. Global variables end up at the top of the LLVM IR in the output, but parameters appear inline.

llvm/test/CodeGen/AArch64/arm64-windows-calls.ll

117 ↗

(On Diff #311938)

Please use CHECK-LABEL directives to ensure that these assembly checks are from the function you intend to match.

[DavidTruby]

Refactor based on review comments

[DavidTruby]

clang/lib/CodeGen/TargetInfo.cpp

5158–5160

I can add tests for this but I think it should go into a separate patch as it's not behaviour changed by this patch

[DavidTruby]

clang/test/CodeGenCXX/microsoft-abi-sret-and-byval.cpp

493

Yes, you're right. I wrote completely the opposite of what I meant here. I have corrected it now!

[DavidTruby]

Switch from CHECK to CHECK-LABEL in tests.

[rnk]

Thank for the update, apologies for not providing these suggestions the first time.

clang/lib/CodeGen/CGCXXABI.cpp

320–321

We still need a better name for this. Something like, isTrivialForAArch64MSVC. This isn't a test for C++14 aggregate-ness, it's the test that a specific compiler uses to decide if a record may be passed in registers.

---

After thinking about this more, making this a virtual CGCXXABI method, as I suggest below, makes more sense, because then the CGCXXABI class doesn't need to expose this very MSVC-specific method, and we don't need to hoist this code out of MicrosoftCXXABI.

clang/lib/CodeGen/TargetInfo.cpp

5123

This comment is stale. We're looking for something more along the lines of, "... check the C++ properties of the record, such as base classes."

5137

Apologies for moving the goalposts, but after re-reading the code, I feel like it would make more sense to make this a CGCXXABI virtual method. This code then would read:

if (!getCXXABI().isPermittedToBeHomogeneousAggregate(CXXRD))
  return false;

IMO it also makes sense to move this up before checking bases.

[DavidTruby]

Refactor based on review

[DavidTruby]

clang/lib/CodeGen/TargetInfo.cpp

5137

I agree this is much cleaner and have made the change based on your suggestion. Thanks for the review!

[peterwaller-arm]

Suggesting a few small tweaks. Otherwise LGTM when @rnk is happy.

clang/test/CodeGenCXX/homogeneous-aggregates.cpp

129 ↗

(On Diff #315042)

147 ↗

(On Diff #315042)

Same comment (from below) about -LABEL for each of these functions. This should be a 'check'-alike and each function header should have a CHECK-LABEL.

llvm/test/CodeGen/AArch64/arm64-windows-calls.ll

117 ↗

(On Diff #311938)

@DavidTruby I think the intent here is:

CHECK-LABEL: < something matching the first line of the function definition >
CHECK: <thing to be checked>

CHECK-LABEL: <next function>

Such that if by some fluke some other function in the interim produces <thing to be checked>, it won't be allowed to match something produced in the subsequent function. This also renders the code safe against this problem if functions are added or moved around. I suggest having a CHECK-LABEL on each function heading, and the code within those functions should be CHECK/CHECK-NEXT as appropriate.

[rnk]

clang/lib/CodeGen/CGCXXABI.h

143

nit: I would group the new virtual method declaration after these two, since it also has to do with argument classification.

clang/lib/CodeGen/MicrosoftCXXABI.cpp

4343

I think x64 vectorcall supports HVAs, so this could change behavior there, unless you check the triple for aarch64.

Following the principle of limiting the scope of the change, this seems like the right condition:

return !CGM.getTarget().getTriple().isAArch64() || isTrivialForAArch64MSVC(CXXRD);

We can follow up with more x64 vectorcall tests later.

[DavidTruby]

Fix tests to use check-label correctly and add proper precondition on aarch64

[rnk]

lgtm, thanks

llvm/test/CodeGen/AArch64/arm64-windows-calls.ll

117 ↗

(On Diff #311938)

Yep. I think you could still go further by adding a trailing colon. FileCheck matches for substrings of a line, so as written, this could match "call copy_pod", which wouldn't be what you want. The risk of that in this test is low, but it's a good best practice.

[peterwaller-arm]

llvm/test/CodeGen/AArch64/arm64-windows-calls.ll

117 ↗

(On Diff #311938)

To echo @rnk, ideally your CHECK-LABEL statements should match the function header, and typically they're placed immediately above the function header, since that's logically what they're matching. The existence of the CHECK-LABEL primitive on every function header constrains the checks to only match within their intended function between the 'bracket' of two CHECK-LABEL statements.

; CHECK-LABEL: define {{.*}}thing
void thing() {
; CHECK: some codegen in thing
// ... do codegen here.
}

clang/lib/CodeGen/CGCXXABI.h

enum RecordArgABI {
     /// and the call machinery must not move it or introduce extra copies.
     RAA_DirectInMemory,
 
     /// Pass it as a pointer to temporary memory.
     RAA_Indirect
   };
 
   /// Returns how an argument of the given record type should be passed.
   virtual RecordArgABI getRecordArgABI(const CXXRecordDecl *RD) const = 0;
 
   /// Returns true if the implicit 'sret' parameter comes after the implicit
   /// 'this' parameter of C++ instance methods.
   virtual bool isSRetParameterAfterThis() const { return false; }
 
   /// Find the LLVM type used to represent the given member pointer
   /// type.
   virtual llvm::Type *
public:
   virtual void emitCXXStructor(GlobalDecl GD) = 0;
 
   /// Load a vtable from This, an object of polymorphic type RD, or from one of
   /// its virtual bases if it does not have its own vtable. Returns the vtable
   /// and the class from which the vtable was loaded.
   virtual std::pair<llvm::Value *, const CXXRecordDecl *>
   LoadVTablePtr(CodeGenFunction &CGF, Address This,
                 const CXXRecordDecl *RD) = 0;
+
+  static bool isCXX14Aggregate(const CXXRecordDecl *RD);
 };
 
 // Create an instance of a C++ ABI class:
 
 /// Creates an Itanium-family ABI.
 CGCXXABI *CreateItaniumCXXABI(CodeGenModule &CGM);
 
 /// Creates a Microsoft-family ABI.

clang/lib/CodeGen/CGCXXABI.cpp

Args.insert(Args.begin() + 1 + i,
                         AddedArgs.Prefix[i].Type));
   }
   for (const auto &arg : AddedArgs.Suffix) {
     Args.add(RValue::get(arg.Value), arg.Type);
   }
   return AddedStructorArgCounts(AddedArgs.Prefix.size(),
                                 AddedArgs.Suffix.size());
 }
+
+bool CGCXXABI::isCXX14Aggregate(const CXXRecordDecl *RD) {
+  // For AArch64, we use the C++14 definition of an aggregate, so we also
+  // check for:
+  //   No private or protected non static data members.
+  //   No base classes
+  //   No virtual functions
+  // Additionally, we need to ensure that there is a trivial copy assignment
+  // operator, a trivial destructor and no user-provided constructors.
+  if (RD->hasProtectedFields() || RD->hasPrivateFields())
+    return false;
+  if (RD->getNumBases() > 0)
+    return false;
+  if (RD->isPolymorphic())
+    return false;
+  if (RD->hasNonTrivialCopyAssignment())
+    return false;
+  for (const CXXConstructorDecl *Ctor : RD->ctors())
+    if (Ctor->isUserProvided())
+      return false;
+  if (RD->hasNonTrivialDestructor())
+    return false;
+  return true;
+}

clang/lib/CodeGen/MicrosoftCXXABI.cpp

static bool isDeletingDtor(GlobalDecl GD) {
   return isa<CXXDestructorDecl>(GD.getDecl()) &&
          GD.getDtorType() == Dtor_Deleting;
 }
 
 bool MicrosoftCXXABI::hasMostDerivedReturn(GlobalDecl GD) const {
   return isDeletingDtor(GD);
 }
 
-static bool isCXX14Aggregate(const CXXRecordDecl *RD) {
-  // For AArch64, we use the C++14 definition of an aggregate, so we also
-  // check for:
-  //   No private or protected non static data members.
-  //   No base classes
-  //   No virtual functions
-  // Additionally, we need to ensure that there is a trivial copy assignment
-  // operator, a trivial destructor and no user-provided constructors.
-  if (RD->hasProtectedFields() || RD->hasPrivateFields())
-    return false;
-  if (RD->getNumBases() > 0)
-    return false;
-  if (RD->isPolymorphic())
-    return false;
-  if (RD->hasNonTrivialCopyAssignment())
-    return false;
-  for (const CXXConstructorDecl *Ctor : RD->ctors())
-    if (Ctor->isUserProvided())
-      return false;
-  if (RD->hasNonTrivialDestructor())
-    return false;
-  return true;
-}
-
 bool MicrosoftCXXABI::classifyReturnType(CGFunctionInfo &FI) const {
   const CXXRecordDecl *RD = FI.getReturnType()->getAsCXXRecordDecl();
   if (!RD)
     return false;
 
   // Normally, the C++ concept of "is trivially copyable" is used to determine
   // if a struct can be returned directly. However, as MSVC and the language
   // have evolved, the definition of "trivially copyable" has changed, while the

 }
 
 std::pair<llvm::Value *, const CXXRecordDecl *>
 MicrosoftCXXABI::LoadVTablePtr(CodeGenFunction &CGF, Address This,
                                const CXXRecordDecl *RD) {
   std::tie(This, std::ignore, RD) =
       performBaseAdjustment(CGF, This, QualType(RD->getTypeForDecl(), 0));
   return {CGF.GetVTablePtr(This, CGM.Int8PtrTy, RD), RD};
 }

clang/lib/CodeGen/TargetInfo.cpp

if (const ConstantArrayType *AT = getContext().getAsConstantArrayType(Ty)) {
     Members *= NElements;
   } else if (const RecordType *RT = Ty->getAs<RecordType>()) {
     const RecordDecl *RD = RT->getDecl();
     if (RD->hasFlexibleArrayMember())
       return false;
 
     Members = 0;
 
     // If this is a C++ record, check the bases first.
     if (const CXXRecordDecl *CXXRD = dyn_cast<CXXRecordDecl>(RD)) {
       for (const auto &I : CXXRD->bases()) {
         // Ignore empty records.
         if (isEmptyRecord(getContext(), I.getType(), true))
           continue;
 
         uint64_t FldMembers;
         if (!isHomogeneousAggregate(I.getType(), Base, FldMembers))
           return false;
 
         Members += FldMembers;
       }
+
+      const auto TT = CGT.getTarget().getTriple();
+      // MSVC Windows on Arm64 considers a type not HFA if it is not an
+      // aggregate according to the C++14 spec. This is not consistent with the
+      // AAPCS64, but is defacto spec on that platform.
+      if (TT.isAArch64() && TT.isWindowsMSVCEnvironment() &&
+          !CGCXXABI::isCXX14Aggregate(CXXRD))
+        return false;
     }
 
     for (const auto *FD : RD->fields()) {
       // Ignore (non-zero arrays of) empty records.
       QualType FT = FD->getType();
       while (const ConstantArrayType *AT =
              getContext().getAsConstantArrayType(FT)) {
         if (AT->getSize().getZExtValue() == 0)
           return false;
         FT = AT->getElementType();
       }
       if (isEmptyRecord(getContext(), FT, true))
         continue;
 
       // For compatibility with GCC, ignore empty bitfields in C++ mode.
       if (getContext().getLangOpts().CPlusPlus &&
           FD->isZeroLengthBitField(getContext()))
         continue;
 
       uint64_t FldMembers;
       if (!isHomogeneousAggregate(FD->getType(), Base, FldMembers))
         return false;
 
       Members = (RD->isUnion() ?
                  std::max(Members, FldMembers) : Members + FldMembers);

clang/test/CodeGenCXX/microsoft-abi-sret-and-byval.cpp

 // WIN32-LABEL: define dso_local x86_thiscallcc void @"?g@C@pr30293@@QAEXXZ"(%"struct.pr30293::C"* {{[^,]*}} %this)
 // WIN32: call x86_thiscallcc %struct.SmallWithDtor* @"??0SmallWithDtor@@QAE@XZ"
 // WIN32: call void @"?h@C@pr30293@@UAAXUSmallWithDtor@@@Z"(<{ i8*, %struct.SmallWithDtor }>* inalloca %{{[^,)]*}})
 // WIN32: declare dso_local void @"?h@C@pr30293@@UAAXUSmallWithDtor@@@Z"(<{ i8*, %struct.SmallWithDtor }>* inalloca)
 
 // WIN64-LABEL: define dso_local void @"?g@C@pr30293@@QEAAXXZ"(%"struct.pr30293::C"* {{[^,]*}} %this)
 // WIN64: declare dso_local void @"?h@C@pr30293@@UEAAXUSmallWithDtor@@@Z"(i8*, i32)
 }
+
+namespace pr47611 {
+// MSVC on Arm includes "isCXX14Aggregate" as part of its definition of
+// Homogeneous Floating-point Aggregate (HFA). Additionally, it has a different
+// handling of C++14 aggregates, which can lead to confusion.
+
+// Pod is a trivial HFA.
+struct Pod {
+  double b[2];
+};
+// Not an aggregate according to C++14 spec => not HFA according to MSVC.
+struct NotCXX14Aggregate {
+  NotCXX14Aggregate();
+  Pod p;
+};
+// NotPod is a C++14 aggregate. But not HFA, because it contains
+// NotCXX14Aggregate (which itself is not HFA because it's not a C++14
+// aggregate).
+struct NotPod {
+  NotCXX14Aggregate x;
+};
+// WOA64-LABEL: define dso_local %"struct.pr47611::Pod" @"?copy@pr47611@@YA?AUPod@1@PEAU21@@Z"(%"struct.pr47611::Pod"* %x)
+Pod copy(Pod *x) { return *x; }  // MSVC: ldp d0,d1,[x0], Clang: ldp d0,d1,[x0]
+// WOA64-LABEL: define dso_local void @"?copy@pr47611@@YA?AUNotCXX14Aggregate@1@PEAU21@@Z"(%"struct.pr47611::NotCXX14Aggregate"* inreg noalias sret(%"struct.pr47611::NotCXX14Aggregate") align 8 %agg.result, %"struct.pr47611::NotCXX14Aggregate"* %x)
+NotCXX14Aggregate copy(NotCXX14Aggregate *x) { return *x; } // MSVC: stp x8,x9,[x0], Clang: str q0,[x0]
+// WOA64-LABEL: define dso_local [2 x i64] @"?copy@pr47611@@YA?AUNotPod@1@PEAU21@@Z"(%"struct.pr47611::NotPod"* %x)
+NotPod copy(NotPod *x) { return *x; }
+};