[MS] Apply `inreg` to AArch64 sret parms on instance methods

[rnk]

Summary

The documentation rules indicate that instance methods should return
large, trivially copyable aggregates via X1/X0 and not X8 as is normally
done when returning such structs from free functions:
https://docs.microsoft.com/en-us/cpp/build/arm64-windows-abi-conventions?view=vs-2019#return-values

Fixes PR47836, a bug in the initial implementation of these rules.

I tried to simplify the logic a bit as well while I'm here.

[efriedma]

clang/lib/CodeGen/MicrosoftCXXABI.cpp

1073

This is just reversing the result of the function, I guess.

1108–1109

I suspect that the IsSizeGreaterThan128() check shouldn't be here. Can we rely on the C ABI rules for that?

For non-AArch64, is it really correct to use isPOD()? That varies based on the language version, no?

1109

isTrivialForABI is only used if isAArch64 is true, so it shouldn't use isTrivialForABI as part of its computation, I think?

[rnk]

clang/lib/CodeGen/MicrosoftCXXABI.cpp

1108–1109

I think you are right, we can remove the size check here, it's already part of what feeds into RD->canPassInRegisters. There is an earlier size check here:
https://github.com/llvm/llvm-project/blob/master/clang/lib/Sema/SemaDeclCXX.cpp#L6441

I agree isPOD is vague, but the comments on the implementation do say this:

/// Note that this is the C++ TR1 definition of POD.

So, I think it doesn't vary in practice.

1109

I see. I think we ended up with this tortured condition because I gave review feedback that it would be good to avoid checking isCXX14Aggregate (formerly hasMicrosoftABIRestritions) for non-aarch64 targets. I'll take another shot at simplifying things.

[rnk]

• simplify some more

[efriedma]

Makes a lot more sense now. LGTM

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 IsSizeGreaterThan128(const CXXRecordDecl *RD) {
-  return RD->getASTContext().getTypeSize(RD->getTypeForDecl()) > 128;
-}
-
-static bool hasMicrosoftABIRestrictions(const CXXRecordDecl *RD) {
+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 true;
+    return false;
   if (RD->getNumBases() > 0)
-    return true;
+    return false;
   if (RD->isPolymorphic())
-    return true;
+    return false;
   if (RD->hasNonTrivialCopyAssignment())
-    return true;
+    return false;
   for (const CXXConstructorDecl *Ctor : RD->ctors())
     if (Ctor->isUserProvided())
-      return true;
+      return false;
   if (RD->hasNonTrivialDestructor())
-    return true;
     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
+  // ABI must remain stable. AArch64 uses the C++14 concept of an "aggregate",
+  // while other ISAs use the older concept of "plain old data".
+  bool isTrivialForABI = RD->isPOD();
   bool isAArch64 = CGM.getTarget().getTriple().isAArch64();
-  bool isSimple = !isAArch64 || !hasMicrosoftABIRestrictions(RD);
-  bool isIndirectReturn =
-      isAArch64 ? (!RD->canPassInRegisters() ||
-                   IsSizeGreaterThan128(RD))
-                : !RD->isPOD();
-  bool isInstanceMethod = FI.isInstanceMethod();
+  if (isAArch64)
+    isTrivialForABI = RD->canPassInRegisters() && isCXX14Aggregate(RD);
 
-  if (isIndirectReturn || !isSimple || isInstanceMethod) {
+  // MSVC always returns structs indirectly from C++ instance methods.
+  bool isIndirectReturn = !isTrivialForABI || FI.isInstanceMethod();
+
+  if (isIndirectReturn) {
     CharUnits Align = CGM.getContext().getTypeAlignInChars(FI.getReturnType());
     FI.getReturnInfo() = ABIArgInfo::getIndirect(Align, /*ByVal=*/false);
-    FI.getReturnInfo().setSRetAfterThis(isInstanceMethod);
 
-    FI.getReturnInfo().setInReg(isAArch64 &&
-                                !(isSimple && IsSizeGreaterThan128(RD)));
+    // MSVC always passes `this` before the `sret` parameter.
+    FI.getReturnInfo().setSRetAfterThis(FI.isInstanceMethod());
+
+    // On AArch64, use the `inreg` attribute if the object is considered to not
+    // be trivially copyable, or if this is an instance method struct return.
+    FI.getReturnInfo().setInReg(isAArch64);
 
     return true;
   }
 
   // Otherwise, use the C ABI rules.
   return false;
 }
 

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

void call_bools_and_chars() {
   take_bools_and_chars('A', 'B', SmallWithDtor(), 'D', true, 13, false);
 }
 
 // Returning structs that fit into a register.
 Small small_return() { return Small(); }
 // LINUX-LABEL: define void @_Z12small_returnv(%struct.Small* noalias sret align 4 %agg.result)
 // WIN32: define dso_local i32 @"?small_return@@YA?AUSmall@@XZ"()
 // WIN64: define dso_local i32 @"?small_return@@YA?AUSmall@@XZ"()
+// WOA64: define dso_local i64 @"?small_return@@YA?AUSmall@@XZ"()
 
 Medium medium_return() { return Medium(); }
 // LINUX-LABEL: define void @_Z13medium_returnv(%struct.Medium* noalias sret align 4 %agg.result)
 // WIN32: define dso_local i64 @"?medium_return@@YA?AUMedium@@XZ"()
 // WIN64: define dso_local i64 @"?medium_return@@YA?AUMedium@@XZ"()
+// WOA64: define dso_local i64 @"?medium_return@@YA?AUMedium@@XZ"()
 
 // Returning structs that fit into a register but are not POD.
 SmallCpp11NotCpp03Pod small_non_pod_return() { return SmallCpp11NotCpp03Pod(); }
 // LINUX-LABEL: define void @_Z20small_non_pod_returnv(%struct.SmallCpp11NotCpp03Pod* noalias sret align 4 %agg.result)
 // WIN32: define dso_local void @"?small_non_pod_return@@YA?AUSmallCpp11NotCpp03Pod@@XZ"(%struct.SmallCpp11NotCpp03Pod* noalias sret align 4 %agg.result)
 // WIN64: define dso_local void @"?small_non_pod_return@@YA?AUSmallCpp11NotCpp03Pod@@XZ"(%struct.SmallCpp11NotCpp03Pod* noalias sret align 4 %agg.result)
+// WOA64: define dso_local void @"?small_non_pod_return@@YA?AUSmallCpp11NotCpp03Pod@@XZ"(%struct.SmallCpp11NotCpp03Pod* inreg noalias sret align 4 %agg.result)
 
 SmallWithCtor small_with_ctor_return() { return SmallWithCtor(); }
 // LINUX-LABEL: define void @_Z22small_with_ctor_returnv(%struct.SmallWithCtor* noalias sret align 4 %agg.result)
 // WIN32: define dso_local void @"?small_with_ctor_return@@YA?AUSmallWithCtor@@XZ"(%struct.SmallWithCtor* noalias sret align 4 %agg.result)
 // WIN64: define dso_local void @"?small_with_ctor_return@@YA?AUSmallWithCtor@@XZ"(%struct.SmallWithCtor* noalias sret align 4 %agg.result)
 // FIXME: The 'sret' mark here doesn't seem to be enough to convince LLVM to
 // preserve the hidden sret pointer in R0 across the function.
 // WOA: define dso_local arm_aapcs_vfpcc void @"?small_with_ctor_return@@YA?AUSmallWithCtor@@XZ"(%struct.SmallWithCtor* noalias sret align 4 %agg.result)
+// WOA64: define dso_local void @"?small_with_ctor_return@@YA?AUSmallWithCtor@@XZ"(%struct.SmallWithCtor* inreg noalias sret align 4 %agg.result)
+
+SmallWithDtor small_with_dtor_return() { return SmallWithDtor(); }
+// LINUX-LABEL: define void @_Z22small_with_dtor_returnv(%struct.SmallWithDtor* noalias sret align 4 %agg.result)
+// WIN32: define dso_local void @"?small_with_dtor_return@@YA?AUSmallWithDtor@@XZ"(%struct.SmallWithDtor* noalias sret align 4 %agg.result)
+// WIN64: define dso_local void @"?small_with_dtor_return@@YA?AUSmallWithDtor@@XZ"(%struct.SmallWithDtor* noalias sret align 4 %agg.result)
+// WOA64: define dso_local void @"?small_with_dtor_return@@YA?AUSmallWithDtor@@XZ"(%struct.SmallWithDtor* inreg noalias sret align 4 %agg.result)
 
 SmallWithVftable small_with_vftable_return() { return SmallWithVftable(); }
 // LINUX-LABEL: define void @_Z25small_with_vftable_returnv(%struct.SmallWithVftable* noalias sret align 4 %agg.result)
 // WIN32: define dso_local void @"?small_with_vftable_return@@YA?AUSmallWithVftable@@XZ"(%struct.SmallWithVftable* noalias sret align 4 %agg.result)
 // WIN64: define dso_local void @"?small_with_vftable_return@@YA?AUSmallWithVftable@@XZ"(%struct.SmallWithVftable* noalias sret align 8 %agg.result)
+// WOA64: define dso_local void @"?small_with_vftable_return@@YA?AUSmallWithVftable@@XZ"(%struct.SmallWithVftable* inreg noalias sret align 8 %agg.result)
 
 MediumWithCopyCtor medium_with_copy_ctor_return() { return MediumWithCopyCtor(); }
 // LINUX-LABEL: define void @_Z28medium_with_copy_ctor_returnv(%struct.MediumWithCopyCtor* noalias sret align 4 %agg.result)
 // WIN32: define dso_local void @"?medium_with_copy_ctor_return@@YA?AUMediumWithCopyCtor@@XZ"(%struct.MediumWithCopyCtor* noalias sret align 4 %agg.result)
 // WIN64: define dso_local void @"?medium_with_copy_ctor_return@@YA?AUMediumWithCopyCtor@@XZ"(%struct.MediumWithCopyCtor* noalias sret align 4 %agg.result)
 // WOA: define dso_local arm_aapcs_vfpcc void @"?medium_with_copy_ctor_return@@YA?AUMediumWithCopyCtor@@XZ"(%struct.MediumWithCopyCtor* noalias sret align 4 %agg.result)
+// WOA64: define dso_local void @"?medium_with_copy_ctor_return@@YA?AUMediumWithCopyCtor@@XZ"(%struct.MediumWithCopyCtor* inreg noalias sret align 4 %agg.result)
 
 // Returning a large struct that doesn't fit into a register.
 Big big_return() { return Big(); }
 // LINUX-LABEL: define void @_Z10big_returnv(%struct.Big* noalias sret align 4 %agg.result)
 // WIN32: define dso_local void @"?big_return@@YA?AUBig@@XZ"(%struct.Big* noalias sret align 4 %agg.result)
 // WIN64: define dso_local void @"?big_return@@YA?AUBig@@XZ"(%struct.Big* noalias sret align 4 %agg.result)
+// WOA64: define dso_local void @"?big_return@@YA?AUBig@@XZ"(%struct.Big* noalias sret align 4 %agg.result)
 
 
 void small_arg(Small s) {}
 // LINUX-LABEL: define void @_Z9small_arg5Small(i32 %s.0)
 // WIN32: define dso_local void @"?small_arg@@YAXUSmall@@@Z"(i32 %s.0)
 // WIN64: define dso_local void @"?small_arg@@YAXUSmall@@@Z"(i32 %s.coerce)
 // WOA: define dso_local arm_aapcs_vfpcc void @"?small_arg@@YAXUSmall@@@Z"([1 x i32] %s.coerce)
 

 // WIN64: call void @"?takes_ref_field@@YAXURefField@@@Z"(i64 %{{.*}})
 
 class Class {
  public:
   Small thiscall_method_small() { return Small(); }
   // LINUX: define {{.*}} void @_ZN5Class21thiscall_method_smallEv(%struct.Small* noalias sret align 4 %agg.result, %class.Class* %this)
   // WIN32: define {{.*}} x86_thiscallcc void @"?thiscall_method_small@Class@@QAE?AUSmall@@XZ"(%class.Class* %this, %struct.Small* noalias sret align 4 %agg.result)
   // WIN64: define linkonce_odr dso_local void @"?thiscall_method_small@Class@@QEAA?AUSmall@@XZ"(%class.Class* %this, %struct.Small* noalias sret align 4 %agg.result)
+  // WOA64: define linkonce_odr dso_local void @"?thiscall_method_small@Class@@QEAA?AUSmall@@XZ"(%class.Class* %this, %struct.Small* inreg noalias sret align 4 %agg.result)
 
   SmallWithCtor thiscall_method_small_with_ctor() { return SmallWithCtor(); }
   // LINUX: define {{.*}} void @_ZN5Class31thiscall_method_small_with_ctorEv(%struct.SmallWithCtor* noalias sret align 4 %agg.result, %class.Class* %this)
   // WIN32: define {{.*}} x86_thiscallcc void @"?thiscall_method_small_with_ctor@Class@@QAE?AUSmallWithCtor@@XZ"(%class.Class* %this, %struct.SmallWithCtor* noalias sret align 4 %agg.result)
   // WIN64: define linkonce_odr dso_local void @"?thiscall_method_small_with_ctor@Class@@QEAA?AUSmallWithCtor@@XZ"(%class.Class* %this, %struct.SmallWithCtor* noalias sret align 4 %agg.result)
+  // WOA64: define linkonce_odr dso_local void @"?thiscall_method_small_with_ctor@Class@@QEAA?AUSmallWithCtor@@XZ"(%class.Class* %this, %struct.SmallWithCtor* inreg noalias sret align 4 %agg.result)
 
   Small __cdecl cdecl_method_small() { return Small(); }
   // LINUX: define {{.*}} void @_ZN5Class18cdecl_method_smallEv(%struct.Small* noalias sret align 4 %agg.result, %class.Class* %this)
   // WIN32: define {{.*}} void @"?cdecl_method_small@Class@@QAA?AUSmall@@XZ"(%class.Class* %this, %struct.Small* noalias sret align 4 %agg.result)
   // WIN64: define linkonce_odr dso_local void @"?cdecl_method_small@Class@@QEAA?AUSmall@@XZ"(%class.Class* %this, %struct.Small* noalias sret align 4 %agg.result)
 
   Big __cdecl cdecl_method_big() { return Big(); }
   // LINUX: define {{.*}} void @_ZN5Class16cdecl_method_bigEv(%struct.Big* noalias sret align 4 %agg.result, %class.Class* %this)
   // WIN32: define {{.*}} void @"?cdecl_method_big@Class@@QAA?AUBig@@XZ"(%class.Class* %this, %struct.Big* noalias sret align 4 %agg.result)
   // WIN64: define linkonce_odr dso_local void @"?cdecl_method_big@Class@@QEAA?AUBig@@XZ"(%class.Class* %this, %struct.Big* noalias sret align 4 %agg.result)
+  // WOA64: define linkonce_odr dso_local void @"?cdecl_method_big@Class@@QEAA?AUBig@@XZ"(%class.Class* %this, %struct.Big* inreg noalias sret align 4 %agg.result)
 
   void thiscall_method_arg(Empty s) {}
   // LINUX: define {{.*}} void @_ZN5Class19thiscall_method_argE5Empty(%class.Class* %this)
   // WIN32: define {{.*}} void @"?thiscall_method_arg@Class@@QAEXUEmpty@@@Z"(%class.Class* %this, %struct.Empty* byval(%struct.Empty) align 4 %s)
   // WIN64: define linkonce_odr dso_local void @"?thiscall_method_arg@Class@@QEAAXUEmpty@@@Z"(%class.Class* %this, i8 %s.coerce)
 
   void thiscall_method_arg(EmptyWithCtor s) {}
   // LINUX: define {{.*}} void @_ZN5Class19thiscall_method_argE13EmptyWithCtor(%class.Class* %this)