[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

1077

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

1108

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

1110

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?

[rnk]

clang/lib/CodeGen/MicrosoftCXXABI.cpp

1108

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.

1110

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.

[rnk]

• simplify some more

[efriedma]

Makes a lot more sense now. LGTM

clang/lib/CodeGen/MicrosoftCXXABI.cpp

 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;
 
   bool isAArch64 = CGM.getTarget().getTriple().isAArch64();
-  bool isSimple = !isAArch64 || !hasMicrosoftABIRestrictions(RD);
+  bool isTrivialForABI =
+      RD->canPassInRegisters() && !(isAArch64 && !isCXX14Aggregate(RD));
   bool isIndirectReturn =
-      isAArch64 ? (!RD->canPassInRegisters() ||
-                   IsSizeGreaterThan128(RD))
-                : !RD->isPOD();
+      isAArch64 ? (!isTrivialForABI || IsSizeGreaterThan128(RD)) : !RD->isPOD();
   bool isInstanceMethod = FI.isInstanceMethod();
 
-  if (isIndirectReturn || !isSimple || isInstanceMethod) {
+  if (isIndirectReturn || isInstanceMethod) {
     CharUnits Align = CGM.getContext().getTypeAlignInChars(FI.getReturnType());
     FI.getReturnInfo() = ABIArgInfo::getIndirect(Align, /*ByVal=*/false);
     FI.getReturnInfo().setSRetAfterThis(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 &&
-                                !(isSimple && IsSizeGreaterThan128(RD)));
+                                (isInstanceMethod || !isTrivialForABI));
 
     return true;
   }
 
   // Otherwise, use the C ABI rules.
   return false;
 }
 

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

 // 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)