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

[Reid Kleckner via Phabricator via cfe-commits]

rnk created this revision.
rnk added reviewers: mgrang, efriedma.
Herald added subscribers: danielkiss, kristof.beyls.
Herald added a project: clang.
rnk requested review of this revision.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89362

Files:
  clang/lib/CodeGen/MicrosoftCXXABI.cpp
  clang/test/CodeGenCXX/microsoft-abi-sret-and-byval.cpp


Index: clang/test/CodeGenCXX/microsoft-abi-sret-and-byval.cpp
===================================================================
--- clang/test/CodeGenCXX/microsoft-abi-sret-and-byval.cpp
+++ clang/test/CodeGenCXX/microsoft-abi-sret-and-byval.cpp
@@ -284,11 +284,13 @@
   // 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 at 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 at 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 at 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 at 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 at 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 at 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)
@@ -299,6 +301,7 @@
   // LINUX: define {{.*}} void @_ZN5Class16cdecl_method_bigEv(%struct.Big* noalias sret align 4 %agg.result, %class.Class* %this)
   // WIN32: define {{.*}} void @"?cdecl_method_big at 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 at 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 at 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)
Index: clang/lib/CodeGen/MicrosoftCXXABI.cpp
===================================================================
--- clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -1074,7 +1074,7 @@
   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.
@@ -1083,19 +1083,19 @@
   // 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 false;
+  return true;
 }
 
 bool MicrosoftCXXABI::classifyReturnType(CGFunctionInfo &FI) const {
@@ -1104,20 +1104,21 @@
     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;
   }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D89362.298017.patch
Type: text/x-patch
Size: 5313 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20201014/f30d478c/attachment.bin>