MSVC AArch64 ABI: Homogeneous aggregates

[dblaikie]

Summary

Fixes:
Protected members, HFA: https://godbolt.org/z/zqdK7vdKc
Private members, HFA: https://godbolt.org/z/zqdK7vdKc
Non-empty base, HFA: https://godbolt.org/z/PKTz59Wev
User-provided ctor, HFA: https://godbolt.org/z/sfrTddcW6

Existing correct cases:
Empty base class, NonHFA: https://godbolt.org/z/4veY9MWP3

• correct by accident of not allowing bases at all (see non-empty base case/fix above for counterexample)

Polymorphic: NonHFA: https://godbolt.org/z/4veY9MWP3
Trivial copy assignment, HFA: https://godbolt.org/z/Tdecj836P
Non-trivial copy assignment, NonHFA: https://godbolt.org/z/7c4bE9Whq
Non-trivial default ctor, NonHFA: https://godbolt.org/z/Tsq1EE7b7

• correct by accident of disallowing all user-provided ctors (see user-provided non-default ctor example above for counterexample)

Trivial dtor, HFA: https://godbolt.org/z/nae999aqz
Non-trivial dtor, NonHFA: https://godbolt.org/z/69oMcshb1
Empty field, NonHFA: https://godbolt.org/z/8PTxsKKMK

• true due to checking for the absence of padding (see comment in code)

After a bunch of testing, this fixes a bunch of cases that were
incorrect. Some of the tests verify the nuances of the existing
behavior/code checks that were already present.

This was mostly motivated by cleanup from/in D133817 which itself was
motivated by D119051.

By removing the incorrect use of isTrivialForAArch64MSVC here & adding
more nuance to the homogeneous testing we can more safely/confidently
make changes to the isTrivialFor(AArch64)MSVC to more properly align
with its usage anyway.

[rnk]

Nice! Mostly comment copy edits.

clang/lib/CodeGen/MicrosoftCXXABI.cpp

4476–4477

These comments seem stale. Are the checks below almost equivalent to C++14 isAggregate? In any case, they associate with the logic below, which is arm-specific.

The arm check deserves some kind of comment to say that all aggregates are permitted to be HFAs for non-ARM platforms, which mostly affects vectorcall on x64/x86.

4494

typo Instnead

4497–4499

There's an open parenthetical here, maybe just make it . For example, if you have...

[dblaikie]

Update comments

[dblaikie]

clang/lib/CodeGen/MicrosoftCXXABI.cpp

4476–4477

Not especially equivalent to C++14 aggregate, despite Microsoft's reports, I think.
For instance - the first requirement of a C++14 aggregate is that it has no user-provided ctors, but that's not a requirement for HFA: https://godbolt.org/z/Whazeha7v
& the no base classes requirement doesn't apply to HFA either - only no non-HFA base classes (& empty classes are considered non-HFA).

Updated the comment as best I could understand (I couldn't actually find the AAPCS64 rules to compare/contrast, so I'm hand waving a bit there - perhaps these ( https://github.com/ARM-software/abi-aa/blob/main/aapcs64/aapcs64.rst#homogeneous-aggregates ) are all the rules, though they're pretty slim, and don't discuss special members at all, for instance, nor refer to any particular C++ standard for further clarification, so far as I can see)

[dblaikie]

Ping on this

[rnk]

lgtm

clang/lib/CodeGen/MicrosoftCXXABI.cpp

 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};
 }
 
 bool MicrosoftCXXABI::isPermittedToBeHomogeneousAggregate(
-    const CXXRecordDecl *CXXRD) const {
-  // 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.
-  return !CGM.getTarget().getTriple().isAArch64() ||
-         isTrivialForAArch64MSVC(CXXRD);
+    const CXXRecordDecl *RD) const {
+  // All aggregates are permitted to be HFA on non-ARM platforms, which mostly
+  // affects vectorcall on x64/x86.
+  if (!CGM.getTarget().getTriple().isAArch64())
+    return true;
+  // MSVC Windows on Arm64 has its own rules for determining if a type is HFA
+  // that are inconsistent with the AAPCS64 ABI. The following are our best
+  // determination of those rules so far, based on observation of MSVC's
+  // behavior.
+  if (RD->isEmpty())
+    return false;
+  if (RD->isPolymorphic())
+    return false;
+  if (RD->hasNonTrivialCopyAssignment())
+    return false;
+  if (RD->hasNonTrivialDestructor())
+    return false;
+  if (RD->hasNonTrivialDefaultConstructor())
+    return false;
+  // These two are somewhat redundant given the caller
+  // (ABIInfo::isHomogeneousAggregate) checks the bases and fields, but that
+  // caller doesn't consider empty bases/fields to be non-homogenous, but it
+  // looks like Microsoft's AArch64 ABI does care about these empty types &
+  // anything containing/derived from one is non-homogeneous.
+  // Instead we could add another CXXABI entry point to query this property and
+  // have ABIInfo::isHomogeneousAggregate use that property.
+  // I don't think any other of the features listed above could be true of a
+  // base/field while not true of the outer struct. For example, if you have a
+  // base/field that has an non-trivial copy assignment/dtor/default ctor, then
+  // the outer struct's corresponding operation must be non-trivial.
+  for (const CXXBaseSpecifier &B : RD->bases()) {
+    if (const CXXRecordDecl *FRD = B.getType()->getAsCXXRecordDecl()) {
+      if (!isPermittedToBeHomogeneousAggregate(FRD))
+        return false;
+    }
+  }
+  // empty fields seem to be caught by the ABIInfo::isHomogeneousAggregate
+  // checking for padding - but maybe there are ways to end up with an empty
+  // field without padding? Not that I know of, so don't check fields here &
+  // rely on the padding check.
+  return true;
 }

clang/test/CodeGenCXX/homogeneous-aggregates.cpp

 struct Float2 { float y; };
 struct HVAWithEmptyBase : Float1, Empty, Float2 { float z; };
 
 // PPC: define{{.*}} void @_Z15with_empty_base16HVAWithEmptyBase([3 x float] %a.coerce)
 // ARM64: define{{.*}} void @_Z15with_empty_base16HVAWithEmptyBase([3 x float] %a.coerce)
 // ARM32: define{{.*}} arm_aapcs_vfpcc void @_Z15with_empty_base16HVAWithEmptyBase(%struct.HVAWithEmptyBase %a.coerce)
 void CC with_empty_base(HVAWithEmptyBase a) {}
 
-// FIXME: MSVC doesn't consider this an HVA because of the empty base.
+// WOA64: define dso_local void @"?with_empty_base@@YAXUHVAWithEmptyBase@@@Z"([2 x i64] %{{.*}})
 // X64: define dso_local x86_vectorcallcc void @"\01_Z15with_empty_base16HVAWithEmptyBase@@16"(%struct.HVAWithEmptyBase inreg %a.coerce)
 
 struct HVAWithEmptyBitField : Float1, Float2 {
   int : 0; // Takes no space.
   float z;
 };
 
 // PPC: define{{.*}} void @_Z19with_empty_bitfield20HVAWithEmptyBitField([3 x float] %a.coerce)
void call_copy_hasemptybase(HasEmptyBase *hasEmptyBase) {
   // WOA64: call void @"?copy@pr47611@@YA?AUHasEmptyBase@1@PEAU21@@Z"(%"struct.pr47611::HasEmptyBase"* inreg sret(%"struct.pr47611::HasEmptyBase") align 8 %{{.*}}, %"struct.pr47611::HasEmptyBase"* noundef %{{.*}})
 }
 
 void call_copy_haspodbase(HasPodBase *hasPodBase) {
   *hasPodBase = copy(hasPodBase);
   // WOA64-LABEL: define dso_local void @"?call_copy_haspodbase@pr47611@@YAXPEAUHasPodBase@1@@Z"
   // WOA64: call void @"?copy@pr47611@@YA?AUHasPodBase@1@PEAU21@@Z"(%"struct.pr47611::HasPodBase"* inreg sret(%"struct.pr47611::HasPodBase") align 8 %{{.*}}, %"struct.pr47611::HasPodBase"* noundef %{{.*}})
 }
-}; // namespace pr47611
+} // namespace pr47611
+
+namespace protected_member {
+struct HFA {
+  double x;
+  double y;
+protected:
+  double z;
+};
+double foo(HFA v) { return v.x + v.y; }
+// WOA64: define dso_local noundef double @"?foo@protected_member@@YANUHFA@1@@Z"([3 x double] %{{.*}})
+}
+namespace private_member {
+struct HFA {
+  double x;
+  double y;
+private:
+  double z;
+};
+double foo(HFA v) { return v.x + v.y; }
+// WOA64: define dso_local noundef double @"?foo@private_member@@YANUHFA@1@@Z"([3 x double] %{{.*}})
+}
+namespace polymorphic {
+struct NonHFA {
+  double x;
+  double y;
+  double z;
+  virtual void f1();
+};
+double foo(NonHFA v) { return v.x + v.y; }
+// WOA64: define dso_local noundef double @"?foo@polymorphic@@YANUNonHFA@1@@Z"(%"struct.polymorphic::NonHFA"* noundef %{{.*}})
+}
+namespace trivial_copy_assignment {
+struct HFA {
+  double x;
+  double y;
+  double z;
+  HFA &operator=(const HFA&) = default;
+};
+double foo(HFA v) { return v.x + v.y; }
+// WOA64: define dso_local noundef double @"?foo@trivial_copy_assignment@@YANUHFA@1@@Z"([3 x double] %{{.*}})
+}
+namespace non_trivial_copy_assignment {
+struct NonHFA {
+  double x;
+  double y;
+  double z;
+  NonHFA &operator=(const NonHFA&);
+};
+double foo(NonHFA v) { return v.x + v.y; }
+// WOA64: define dso_local noundef double @"?foo@non_trivial_copy_assignment@@YANUNonHFA@1@@Z"(%"struct.non_trivial_copy_assignment::NonHFA"* noundef %{{.*}})
+}
+namespace user_provided_ctor {
+struct HFA {
+  double x;
+  double y;
+  double z;
+  HFA(int);
+};
+double foo(HFA v) { return v.x + v.y; }
+// WOA64: define dso_local noundef double @"?foo@user_provided_ctor@@YANUHFA@1@@Z"([3 x double] %{{.*}})
+}
+namespace trivial_dtor {
+struct HFA {
+  double x;
+  double y;
+  double z;
+  ~HFA() = default;
+};
+double foo(HFA v) { return v.x + v.y; }
+// WOA64: define dso_local noundef double @"?foo@trivial_dtor@@YANUHFA@1@@Z"([3 x double] %{{.*}})
+}
+namespace non_trivial_dtor {
+struct NonHFA {
+  double x;
+  double y;
+  double z;
+  ~NonHFA();
+};
+double foo(NonHFA v) { return v.x + v.y; }
+// WOA64: define dso_local noundef double @"?foo@non_trivial_dtor@@YANUNonHFA@1@@Z"(%"struct.non_trivial_dtor::NonHFA"* noundef %{{.*}})
+}
+namespace non_empty_base {
+struct non_empty_base { double d; };
+struct HFA : non_empty_base {
+  double x;
+  double y;
+  double z;
+};
+double foo(HFA v) { return v.x + v.y; }
+// WOA64: define dso_local noundef double @"?foo@non_empty_base@@YANUHFA@1@@Z"([4 x double] %{{.*}})
+}
+namespace empty_field {
+struct empty { };
+struct NonHFA {
+  double x;
+  double y;
+  double z;
+  empty e;
+};
+double foo(NonHFA v) { return v.x + v.y; }
+// WOA64: define dso_local noundef double @"?foo@empty_field@@YANUNonHFA@1@@Z"(%"struct.empty_field::NonHFA"* noundef %{{.*}})
+}
+namespace non_empty_field {
+struct non_empty { double d; };
+struct HFA {
+  double x;
+  double y;
+  double z;
+  non_empty e;
+};
+double foo(HFA v) { return v.x + v.y; }
+// WOA64: define dso_local noundef double @"?foo@non_empty_field@@YANUHFA@1@@Z"([4 x double] %{{.*}})
+}