[COFF, ARM64] Fix ABI implementation of struct returns

[mgrang]

Summary

Related llvm patch: D60348.
Patch co-authored by Sanjin Sijaric.

[mgrang]

Got rid of the confusing SuppressSRet logic. The "inreg" attribute is now used to indicate sret returns in X0.

[ostannard]

The document you linked in the LLVM change (https://docs.microsoft.com/en-us/cpp/build/arm64-windows-abi-conventions?view=vs-2019#return-values) says that small POD types are returned directly in X0 or X0 and X1, but this looks like it will always return them indirectly. I think we also need to check the size of the type, and fall back the the plain C ABI for small types.

lib/CodeGen/MicrosoftCXXABI.cpp

1095

Nit: this will use X1 for functions with a this parameter, not X0.

1097

This should also check aarch64_be.

[efriedma]

lib/CodeGen/MicrosoftCXXABI.cpp

1097

aarch64_be-windows isn't a thing; it doesn't make sense to check for that explicitly.

That said, there is an isAArch64() helper which is probably more readable anyway.

[mgrang]

In D60349#1462638, @ostannard wrote:

The document you linked in the LLVM change (https://docs.microsoft.com/en-us/cpp/build/arm64-windows-abi-conventions?view=vs-2019#return-values) says that small POD types are returned directly in X0 or X0 and X1, but this looks like it will always return them indirectly. I think we also need to check the size of the type, and fall back the the plain C ABI for small types.

[richard.townsend.arm]

lib/CodeGen/MicrosoftCXXABI.cpp

1093

Microsoft have updated the spec since this was written, it now says to check for aggregate-ness, trivial copy, and trivial destruct, instead of POD.

[ssijaric]

lib/CodeGen/MicrosoftCXXABI.cpp

1093

This is only true for aggregates. We can have non-aggregate return types that are less or equal to 16 bytes. In this case, they are returned on the stack.

1110

The size returned is in bits, not bytes.

As mentioned above, this applies to aggregates with trivial copy assignment operators and destructors. I will provide a function to check for this. At that point, the check can be removed from here, and the check below can be replaced with something like:

bool isIndirectReturn = !canReturnInRegisters();

[richard.townsend.arm]

lib/CodeGen/MicrosoftCXXABI.cpp

1104

Based on https://reviews.llvm.org/D60348 - lib/Target/AArch64/AArch64CallingConvention.td (line 44), should this be (isIndirectReturn || isInstanceMethod)?

[efriedma]

lib/Sema/SemaDeclCXX.cpp

5906

The isAggregate predicate is not appropriate to check here; it varies depending on whether the language version is C++14 or C++17. For example, C++17 aggregates are allowed to have base classes. You have to split out checking the relevant properties (getNumBases(), isDynamicClass(), etc.).

[efriedma]

lib/Sema/SemaDeclCXX.cpp

5904

I'm not entirely sure it makes sense to do all of these Windows-specific checks here... I think it makes trivial_abi not work the way it should. But that's not something we need to worry about for the initial patch, I think.

5916

D->getAccess() is the access specifier for the class itself (if the class is nested inside another class), not the access specifier for any of its members.

I think you're looking for HasProtectedFields and HasPrivateFields. (I think there currently isn't any getter on CXXRecordDecl for them at the moment, but you can add one.)

5922

Why are you checking needsImplicitCopyAssignment()?

[richard.townsend.arm]

lib/Sema/SemaDeclCXX.cpp

5908

I think these checks are in the wrong place - the aggregate restriction applies to return values, but it doesn't apply to arguments. Placing this logic here breaks argument passing between MSVC and Clang, because Clang (with this logic in place) will pass and expect arguments with an extra layer of indirection: for example, when passing https://cs.chromium.org/chromium/src/v8/include/v8.h?q=v8::Local&sq=package:chromium&g=0&l=183 v8::Local, what you actually see in the register bank is supposed to be its only private member. Moving the logic to MicrosoftCXXABI and applying it only to the return value resolves this problem.

5916

So the issue that I mentioned in https://bugs.llvm.org/show_bug.cgi?id=41135#c18 is resolved by checking HasProtectedFields/HasPrivateFields.

[efriedma]

It looks like there's some missing documentation in the ARM64 ABI document involving homogeneous aggregates... in particular, it looks like non-POD types are never homogeneous, or something along those lines. I guess we can address that in a followup, though.

@TomTan could you look into updating the ARM64 ABI documentation?

Testcase:

struct Pod {
  double b[2];
};
struct NotAggregate {
  NotAggregate();
  double b[2];
};
struct NotPod {
  NotAggregate x;
};
Pod copy(Pod *x) { return *x; }  // ldp d0,d1,[x0]
NotAggregate copy(NotAggregate *x) { return *x; } // stp x8,x9,[x0]
NotPod copy(NotPod *x) { return *x; } // ldp x0,x1,[x8]

lib/Sema/SemaDeclCXX.cpp

5959

just did some quick tests; I think this rule does in fact apply to AArch64, except that the limit is 128 bits, instead of 64 bits.

[TomTan]

In D60349#1476371, @efriedma wrote:

It looks like there's some missing documentation in the ARM64 ABI document involving homogeneous aggregates... in particular, it looks like non-POD types are never homogeneous, or something along those lines. I guess we can address that in a followup, though.

@TomTan could you look into updating the ARM64 ABI documentation?

Testcase:

struct Pod {
  double b[2];
};
struct NotAggregate {
  NotAggregate();
  double b[2];
};
struct NotPod {
  NotAggregate x;
};
Pod copy(Pod *x) { return *x; }  // ldp d0,d1,[x0]
NotAggregate copy(NotAggregate *x) { return *x; } // stp x8,x9,[x0]
NotPod copy(NotPod *x) { return *x; } // ldp x0,x1,[x8]

@efriedma from your test sample above, which part is missing in the ARM64 ABI document? For NotPod, it is aggregate which is specific in the document.

[efriedma]

For NotPod, it is aggregate which is specific in the document

Yes, it's an aggregate which is returned in registers... but it's returned in integer registers, unlike Pod which is returned in floating-point registers.

[TomTan]

In D60349#1477183, @efriedma wrote:

For NotPod, it is aggregate which is specific in the document

Yes, it's an aggregate which is returned in registers... but it's returned in integer registers, unlike Pod which is returned in floating-point registers.

struct Pod is HFA (Homogenous Floating-Point Aggregates) which is returned in floating-point register, struct NotPod is not HFA so still returned in integer registers. The ARM64 ABI document will be updated to reflect this, thanks.

[richard.townsend.arm]

lib/CodeGen/MicrosoftCXXABI.cpp

1074

Should this function also check for user-provided constructors?

[richard.townsend.arm]

lib/CodeGen/MicrosoftCXXABI.cpp

1074

I think it should: I speculatively added these two lines

if (RD->hasUserDeclaredConstructor())
  return true;

and it resolved the problem with std::setw I mentioned in the bug tracker (which means Electron could start).

[efriedma]

lib/CodeGen/MicrosoftCXXABI.cpp

1074

The comment says it's supposed to check for user-provided constructors, but there isn't any code to perform that check, yes.

hasUserDeclaredConstructor() isn't precisely correct; from the C++ standard, "A function is user-provided if it is user-declared and not explicitly defaulted or deleted on its first declaration."

[TomTan]

In D60349#1477770, @TomTan wrote:

In D60349#1477183, @efriedma wrote:

For NotPod, it is aggregate which is specific in the document

Yes, it's an aggregate which is returned in registers... but it's returned in integer registers, unlike Pod which is returned in floating-point registers.

struct Pod is HFA (Homogenous Floating-Point Aggregates) which is returned in floating-point register, struct NotPod is not HFA so still returned in integer registers. The ARM64 ABI document will be updated to reflect this, thanks.

@efriedma Return info for HFA and HVA is updated in https://github.com/MicrosoftDocs/cpp-docs/pull/970.

[efriedma]

Return info for HFA and HVA is updated

That's helpful, but not really sufficient; according to the AAPCS rules, both "Pod" and "NotPod" from my testcase are HFAs.

[TomTan]

In D60349#1479604, @efriedma wrote:

Return info for HFA and HVA is updated

That's helpful, but not really sufficient; according to the AAPCS rules, both "Pod" and "NotPod" from my testcase are HFAs.

Could you provide some more details on why NotPod is HFA, according to AAPCS?

[efriedma]

Could you provide some more details on why NotPod is HFA, according to AAPCS?

In general, computing whether a composite type is a "homogeneous aggregate" according to section 4.3.5 depends only on the "fundamental data types". It looks through "aggregates" (C structs/C++ classes), unions, and arrays. The test applies "without regard to access control or other source language restrictions". Section 7.1.6 mentions there are additional rules that apply to non-POD structs, but that's just referring to the Itanium C++ ABI rule that types which are "non-trivial for the purposes of calls" are passed and returned indirectly. Both clang and gcc agree that "NotPod" is an HFA on AArch64 Linux.

So the question is, what rule is MSVC using to exclude "NotPod" from being an HFA?

[efriedma]

lib/CodeGen/MicrosoftCXXABI.cpp

1078

Like I mentioned before, the rule says "user-provided", not "user-declared".

1080

The rule says "trivial" not "user-declared".

[richard.townsend.arm]

Confirmed just now that the current diff (196774) does work, but it'd be good to correct user-provided constructors and trivial destructors before this lands.

[richard.townsend.arm]

The current diff (196894) seems to have the same std::setw issue as the previous one. Here's what it's trying to compile:

_MRTIMP2 _Smanip<streamsize> __cdecl setw(streamsize wide)
{	// manipulator to set width
      return (_Smanip<streamsize>(&swfun, wide));
 }

Here's the definition for _Smanip:

  template<class _Arg>
    struct _Smanip
      {	// store function pointer and argument value
        _Smanip(void (__cdecl *_Left)(ios_base&, _Arg), _Arg _Val)
		: _Pfun(_Left), _Manarg(_Val)
		{	// construct from function pointer and argument value
		}

               void (__cdecl *_Pfun)(ios_base&, _Arg);	// the function pointer
                   _Arg _Manarg;	// the argument value
     };

[richard.townsend.arm]

Apologies, I meant to write "here's what Clang is trying to call" in the previous comment. It's clear from looking at MSVC's output that MSVC expects to return indirectly via X0, implying that _Smanip is not aggregate by MSVC's definition.

[richard.townsend.arm]

lib/CodeGen/MicrosoftCXXABI.cpp

1080

So I think that the problem with this new check is that it doesn't check all of the constructors. I replaced it with

for (auto it = RD->ctor_begin(); it != RD->ctor_end(); ++it) {
  if (it->isUserProvided())
    return true;
}

And that seems to resolve the setw problem.

[mgrang]

Thanks @richard.townsend.arm . I have updated the patch.

[efriedma]

I'd like to see a few more tests to cover the interesting cases that came up during development of this patch:

1. The user-provided constructor issue: there should be testcases with an explicit user-provided constructor, a non-trivial constructor that's explicitly defaulted, and a non-trivial constructor that's implicitly defaulted.
2. The difference between a user-defined and a non-trivial destructor: there should be a testcase with an implicit non-trivial destructor (which is non-trivial because a member has a non-trivial destructor).
3. A testcase with a trivial copy constructor, but a non-trivial copy-assignment operator.
4. A testcase with a struct with a base class.

[richard.townsend.arm]

lib/CodeGen/MicrosoftCXXABI.cpp

1105

I'm not sure what the IsSizeGreaterThan128 check is doing here - if the return type is over 128 bits, then it will be indirectly returned in X8 with this check, which is not always what we want (e.g. in https://bugs.llvm.org/show_bug.cgi?id=41135 ostream.cpp, MSVC expects the value returned in X1). Another check of hasMicrosoftABIRestrictions might be OK, but that's also not quite right due to the size check.

[efriedma]

lib/CodeGen/MicrosoftCXXABI.cpp

1105

I think the check here is intended to counteract the similar check in hasMicrosoftABIRestrictions... but yes, I don't think that works correctly. Looks like we're missing a testcase for a non-aggregate type with size greater than 128 bits.

[richard.townsend.arm]

lib/CodeGen/MicrosoftCXXABI.cpp

1105

Ah, OK. I think the problem is that we need to check whether the return value meets the rest of aggregate definition (i.e. !hasMicrosoftABIRestrictions), as well as being more than 128 bits in size. May be worth splitting the logic up. Will experiment.

[efriedma]

lib/CodeGen/MicrosoftCXXABI.cpp

1105

I think you might get the right behavior by just erasing both IsSizeGreaterThan128 checks. That matches the way the ABI document describes the rules. Haven't tried it, though.

[rnk]

lib/CodeGen/MicrosoftCXXABI.cpp

55–56

These are both trivial wrappers for RD->canPassInRegisters() which has the real logic. I would just call that to remove some indirection, even if the name is the logical opposite of the property we're trying to check.

1079

Can this be just: for (const CXXConstructorDecl *Ctor : RD->ctors())?

[richard.townsend.arm]

lib/CodeGen/MicrosoftCXXABI.cpp

1105

So I tried that, and everything mostly worked, but there are still some crashes in electron. I think the solution will be something like:

1. Remove the size restriction from the hasMicrosoftABIRestrictions and just decide if the result is aggregate/not aggregate according to the ABI spec.
2. Replace the check with !(isAggregate && IsSizeGreaterThan128(RD))

If both of those things are are true (i.e. it's an aggregate more than >128bits) we should end up calling FI.getReturnInfo().setInReg(false), which implies a return in x8. I haven't confirmed that this works yet because I've run out of time today, but the result should be in tomorrow. May need similar logic on the outer if.

[richard.townsend.arm]

And with those modifications, everything seems to work correctly. I'd be fine with the LLVM portion landing at this stage, but one more rev and another test cycle for this patch would be ideal.

lib/CodeGen/MicrosoftCXXABI.cpp

1085

So... to get the latest diff working, I had to remove this check...

1095

I also had to amend this to:

bool isIndirectReturn = isAArch64 ?
    (passClassIndirect(RD) || hasMicrosoftABIRestrictions(RD)
       || IsSizeGreaterThan128(RD)) :
    !RD->isPOD();

1105

... and also amend this to FI.getReturnInfo().setInReg(isAArch64 && !(isAggregate && IsSizeGreaterThan128(RD)));

[efriedma]

LGTM, assuming it passes testing on electron

[mgrang]

In D60349#1488411, @efriedma wrote:

LGTM, assuming it passes testing on electron

Thanks Eli. @richard.townsend.arm Can you please confirm whether Electron works with this patch and D60348?

[richard.townsend.arm]

Just completed testing... everything seems to be working correctly. So long as the all the tests pass, let's commit! :D

include/clang/AST/DeclCXX.h

public:
   /// The CXXRecordDecl is a class type, but not a union type,
   /// with no non-static data members other than bit-fields of length 0,
   /// no virtual member functions, no virtual base classes,
   /// and no base class B for which is_empty<B>::value is false.
   ///
   /// \note This does NOT include a check for union-ness.
   bool isEmpty() const { return data().Empty; }
 
+  bool hasPrivateFields() const {
+    return data().HasPrivateFields;
+  }
+
+  bool hasProtectedFields() const {
+    return data().HasProtectedFields;
+  }
+
   /// Determine whether this class has direct non-static data members.
   bool hasDirectFields() const {
     auto &D = data();
     return D.HasPublicFields || D.HasProtectedFields || D.HasPrivateFields;
   }
 
   /// Whether this class is polymorphic (C++ [class.virtual]),
   /// which means that the class contains or inherits a virtual function.

include/clang/CodeGen/CGFunctionInfo.h

private:
   bool PaddingInReg : 1;
   bool InAllocaSRet : 1;    // isInAlloca()
   bool IndirectByVal : 1;   // isIndirect()
   bool IndirectRealign : 1; // isIndirect()
   bool SRetAfterThis : 1;   // isIndirect()
   bool InReg : 1;           // isDirect() || isExtend() || isIndirect()
   bool CanBeFlattened: 1;   // isDirect()
   bool SignExt : 1;         // isExtend()
-  bool SuppressSRet : 1;    // isIndirect()
 
   bool canHavePaddingType() const {
     return isDirect() || isExtend() || isIndirect() || isExpand();
   }
   void setPaddingType(llvm::Type *T) {
     assert(canHavePaddingType());
     PaddingType = T;
   }
 
   void setUnpaddedCoerceToType(llvm::Type *T) {
     assert(isCoerceAndExpand());
     UnpaddedCoerceAndExpandType = T;
   }
 
   ABIArgInfo(Kind K)
-      : TheKind(K), PaddingInReg(false), InReg(false), SuppressSRet(false) {
+      : TheKind(K), PaddingInReg(false), InReg(false) {
   }
 
 public:
   ABIArgInfo()
       : TypeData(nullptr), PaddingType(nullptr), DirectOffset(0),
-        TheKind(Direct), PaddingInReg(false), InReg(false),
-        SuppressSRet(false) {}
+        TheKind(Direct), PaddingInReg(false), InReg(false) {}
 
   static ABIArgInfo getDirect(llvm::Type *T = nullptr, unsigned Offset = 0,
                               llvm::Type *Padding = nullptr,
                               bool CanBeFlattened = true) {
     auto AI = ABIArgInfo(Direct);
     AI.setCoerceToType(T);
     AI.setPaddingType(Padding);
     AI.setDirectOffset(Offset);
bool getCanBeFlattened() const {
     return CanBeFlattened;
   }
 
   void setCanBeFlattened(bool Flatten) {
     assert(isDirect() && "Invalid kind!");
     CanBeFlattened = Flatten;
   }
 
-  bool getSuppressSRet() const {
-    assert(isIndirect() && "Invalid kind!");
-    return SuppressSRet;
-  }
-
-  void setSuppressSRet(bool Suppress) {
-    assert(isIndirect() && "Invalid kind!");
-    SuppressSRet = Suppress;
-  }
-
   void dump() const;
 };
 
 /// A class for recording the number of arguments that a function
 /// signature requires.
 class RequiredArgs {
   /// The number of required arguments, or ~0 if the signature does
   /// not permit optional arguments.

lib/CodeGen/CGCall.cpp

void CodeGenModule::ConstructAttributeList(
   }
 
   bool hasUsedSRet = false;
   SmallVector<llvm::AttributeSet, 4> ArgAttrs(IRFunctionArgs.totalIRArgs());
 
   // Attach attributes to sret.
   if (IRFunctionArgs.hasSRetArg()) {
     llvm::AttrBuilder SRETAttrs;
-    if (!RetAI.getSuppressSRet())
-      SRETAttrs.addAttribute(llvm::Attribute::StructRet);
     hasUsedSRet = true;
     if (RetAI.getInReg())
       SRETAttrs.addAttribute(llvm::Attribute::InReg);
     ArgAttrs[IRFunctionArgs.getSRetArgNo()] =
         llvm::AttributeSet::get(getLLVMContext(), SRETAttrs);
   }
 
   // Attach attributes to inalloca argument.

lib/CodeGen/MicrosoftCXXABI.cpp

MicrosoftCXXABI(CodeGenModule &CGM)
         CompleteObjectLocatorType(nullptr), CatchableTypeType(nullptr),
         ThrowInfoType(nullptr) {}
 
   bool HasThisReturn(GlobalDecl GD) const override;
   bool hasMostDerivedReturn(GlobalDecl GD) const override;
 
   bool classifyReturnType(CGFunctionInfo &FI) const override;
 
+  bool passClassIndirect(const CXXRecordDecl *RD) const {
+    return !canCopyArgument(RD);
+  }
+
   RecordArgABI getRecordArgABI(const CXXRecordDecl *RD) const override;
 
   bool isSRetParameterAfterThis() const override { return true; }
 
   bool isThisCompleteObject(GlobalDecl GD) const override {
     // The Microsoft ABI doesn't use separate complete-object vs.
     // base-object variants of constructors, but it does of destructors.
     if (isa<CXXDestructorDecl>(GD.getDecl())) {
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) {
+  // 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, no user-provided constructor, has trivial destructor and type
+  // size <= 128.
+  if (RD->hasProtectedFields() || RD->hasPrivateFields())
+    return true;
+  if (RD->getNumBases() > 0)
+    return true;
+  if (RD->isPolymorphic())
+    return true;
+  if (RD->hasNonTrivialCopyAssignment())
+    return true;
+  if (const auto *Constructor = dyn_cast<CXXConstructorDecl>(RD))
+    if (Constructor->isUserProvided())
+      return true;
+  if (RD->hasNonTrivialDestructor())
+    return true;
+  if (IsSizeGreaterThan128(RD))
+    return true;
+  return false;
+}
+
 bool MicrosoftCXXABI::classifyReturnType(CGFunctionInfo &FI) const {
   const CXXRecordDecl *RD = FI.getReturnType()->getAsCXXRecordDecl();
   if (!RD)
     return false;
 
+  bool isAArch64 = CGM.getTarget().getTriple().isAArch64();
+  bool isIndirectReturn = isAArch64 ?
+    (passClassIndirect(RD) || hasMicrosoftABIRestrictions(RD)) :
+    !RD->isPOD();
+  bool isInstanceMethod = FI.isInstanceMethod();
+
+  if (isIndirectReturn || isInstanceMethod) {
     CharUnits Align = CGM.getContext().getTypeAlignInChars(FI.getReturnType());
-  if (FI.isInstanceMethod()) {
-    // If it's an instance method, aggregates are always returned indirectly via
-    // the second parameter.
     FI.getReturnInfo() = ABIArgInfo::getIndirect(Align, /*ByVal=*/false);
-    FI.getReturnInfo().setSRetAfterThis(FI.isInstanceMethod());
+    FI.getReturnInfo().setSRetAfterThis(isInstanceMethod);
 
-    // aarch64-windows requires that instance methods use X1 for the return
-    // address. So for aarch64-windows we do not mark the
-    // return as SRet.
-    FI.getReturnInfo().setSuppressSRet(CGM.getTarget().getTriple().getArch() ==
-                                       llvm::Triple::aarch64);
-    return true;
-  } else if (!RD->isPOD()) {
-    // If it's a free function, non-POD types are returned indirectly.
-    FI.getReturnInfo() = ABIArgInfo::getIndirect(Align, /*ByVal=*/false);
+    FI.getReturnInfo().setInReg(isAArch64 && !IsSizeGreaterThan128(RD));
 
-    // aarch64-windows requires that non-POD, non-instance returns use X0 for
-    // the return address. So for aarch64-windows we do not mark the return as
-    // SRet.
-    FI.getReturnInfo().setSuppressSRet(CGM.getTarget().getTriple().getArch() ==
-                                       llvm::Triple::aarch64);
     return true;
   }
 
   // Otherwise, use the C ABI rules.
   return false;
 }
 
 llvm::BasicBlock *
 MicrosoftCXXABI::EmitCtorCompleteObjectHandler(CodeGenFunction &CGF,
                                                const CXXRecordDecl *RD) {
   llvm::Value *IsMostDerivedClass = getStructorImplicitParamValue(CGF);
   assert(IsMostDerivedClass &&

lib/Sema/SemaDeclCXX.cpp

if (D->isDependentType() || D->isInvalidDecl())
     return false;
 
   // Clang <= 4 used the pre-C++11 rule, which ignores move operations.
   // The PS4 platform ABI follows the behavior of Clang 3.2.
   if (CCK == TargetInfo::CCK_ClangABI4OrPS4)
     return !D->hasNonTrivialDestructorForCall() &&
            !D->hasNonTrivialCopyConstructorForCall();
 
   if (CCK == TargetInfo::CCK_MicrosoftWin64) {
     bool CopyCtorIsTrivial = false, CopyCtorIsTrivialForCall = false;
     bool DtorIsTrivialForCall = false;
 
     // If a class has at least one non-deleted, trivial copy constructor, it
     // is passed according to the C ABI. Otherwise, it is passed indirectly.
     //
     // Note: This permits classes with non-trivial copy or move ctors to be
     // passed in registers, so long as they *also* have a trivial copy ctor,
     // which is non-conforming.
     if (D->needsImplicitCopyConstructor()) {
       if (!D->defaultedCopyConstructorIsDeleted()) {
         if (D->hasTrivialCopyConstructor())
           CopyCtorIsTrivial = true;
         if (D->hasTrivialCopyConstructorForCall())
           CopyCtorIsTrivialForCall = true;
       }
     } else {
       for (const CXXConstructorDecl *CD : D->ctors()) {
         if (CD->isCopyConstructor() && !CD->isDeleted()) {
           if (CD->isTrivial())
             CopyCtorIsTrivial = true;
           if (CD->isTrivialForCall())
             CopyCtorIsTrivialForCall = true;
         }
       }
     }
if (CCK == TargetInfo::CCK_MicrosoftWin64) {
     // impossible for small types, which it will pass in a single register or
     // stack slot. Most objects with dtors are large-ish, so handle that early.
     // We can't call out all large objects as being indirect because there are
     // multiple x64 calling conventions and the C++ ABI code shouldn't dictate
     // how we pass large POD types.
 
     // Note: This permits small classes with nontrivial destructors to be
     // passed in registers, which is non-conforming.
+    bool isAArch64 = S.Context.getTargetInfo().getTriple().isAArch64();
+    uint64_t TypeSize = isAArch64 ? 128 : 64;
+
     if (CopyCtorIsTrivial &&
-        S.getASTContext().getTypeSize(D->getTypeForDecl()) <= 64)
+        S.getASTContext().getTypeSize(D->getTypeForDecl()) <= TypeSize)
       return true;
     return false;
   }
 
   // Per C++ [class.temporary]p3, the relevant condition is:
   //   each copy constructor, move constructor, and destructor of X is
   //   either trivial or deleted, and X has at least one non-deleted copy
   //   or move constructor

test/CodeGen/arm64-microsoft-arguments.cpp

 // RUN: %clang_cc1 -triple aarch64-windows -ffreestanding -emit-llvm -O0 \
 // RUN: -x c++ -o - %s | FileCheck %s
 
-struct pod { int a, b, c, d, e; };
+// Pass and return for type size <= 8 bytes.
+// CHECK: define {{.*}} i64 @{{.*}}f1{{.*}}()
+// CHECK: call i64 {{.*}}func1{{.*}}(i64 %3)
+struct S1 {
+  int a[2];
+};
 
-struct non_pod {
-  int a;
-  non_pod() {}
+S1 func1(S1 x);
+S1 f1() {
+  S1 x;
+  return func1(x);
+}
+
+// Pass and return type size <= 16 bytes.
+// CHECK: define {{.*}} [2 x i64] @{{.*}}f2{{.*}}()
+// CHECK: call [2 x i64] {{.*}}func2{{.*}}([2 x i64] %3)
+struct S2 {
+  int a[4];
 };
 
-struct pod s;
-struct non_pod t;
+S2 func2(S2 x);
+S2 f2() {
+  S2 x;
+  return func2(x);
+}
+
+// Pass and return for type size > 16 bytes.
+// CHECK: define {{.*}} void @{{.*}}f3{{.*}}(%struct.S3* noalias sret %agg.result)
+// CHECK: call void {{.*}}func3{{.*}}(%struct.S3* sret %agg.result, %struct.S3* %agg.tmp)
+struct S3 {
+  int a[5];
+};
 
-struct pod bar() { return s; }
-struct non_pod foo() { return t; }
-// CHECK: define {{.*}} void @{{.*}}bar{{.*}}(%struct.pod* noalias sret %agg.result)
-// CHECK: define {{.*}} void @{{.*}}foo{{.*}}(%struct.non_pod* noalias %agg.result)
+S3 func3(S3 x);
+S3 f3() {
+  S3 x;
+  return func3(x);
+}
+
+// Pass and return aggregate (of size < 16 bytes) with non-trivial destructor.
+// Passed directly but returned indirectly.
+// CHECK: define {{.*}} void {{.*}}f4{{.*}}(%struct.S4* inreg noalias sret %agg.result)
+// CHECK: call void {{.*}}func4{{.*}}(%struct.S4* inreg sret %agg.result, [2 x i64] %4)
+struct S4 {
+  int a[3];
+  ~S4();
+};
 
+S4 func4(S4 x);
+S4 f4() {
+  S4 x;
+  return func4(x);
+}
+
+// Pass and return from instance method called from instance method.
+// CHECK: define {{.*}} void @{{.*}}bar@Q1{{.*}}(%class.Q1* %this, %class.P1* inreg noalias sret %agg.result)
+// CHECK: call void {{.*}}foo@P1{{.*}}(%class.P1* %ref.tmp, %class.P1* inreg sret %agg.result, i8 %0)
+
+class P1 {
+public:
+  P1 foo(P1 x);
+};
 
-// Check instance methods.
-struct pod2 { int x; };
-struct Baz { pod2 baz(); };
+class Q1 {
+public:
+  P1 bar();
+};
+
+P1 Q1::bar() {
+  P1 p1;
+  return P1().foo(p1);
+}
+
+// Pass and return from instance method called from free function.
+// CHECK: define {{.*}} void {{.*}}bar{{.*}}()
+// CHECK: call void {{.*}}foo@P2{{.*}}(%class.P2* %ref.tmp, %class.P2* inreg sret %retval, i8 %0)
+class P2 {
+public:
+  P2 foo(P2 x);
+};
 
-int qux() { return Baz().baz().x; }
-// CHECK: declare {{.*}} void @{{.*}}baz@Baz{{.*}}(%struct.Baz*, %struct.pod2*)
+P2 bar() {
+  P2 p2;
+  return P2().foo(p2);
+}

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

struct BigWithDtor {
   ~BigWithDtor();
   int a, b, c, d, e, f;
 };
 
 struct BaseNoByval : Small {
   int bb;
 };
 
+struct SmallWithPrivate {
+private:
+ int i;
+};
+
 // WIN32: declare dso_local void @"{{.*take_bools_and_chars.*}}"
 // WIN32:       (<{ i8, [3 x i8], i8, [3 x i8], %struct.SmallWithDtor,
 // WIN32:           i8, [3 x i8], i8, [3 x i8], i32, i8, [3 x i8] }>* inalloca)
 void take_bools_and_chars(char a, char b, SmallWithDtor c, char d, bool e, int f, bool g);
 void call_bools_and_chars() {
   take_bools_and_chars('A', 'B', SmallWithDtor(), 'D', true, 13, false);
 }
 

 void small_arg_with_dtor(SmallWithDtor s) {}
 // WIN32: define dso_local void @"?small_arg_with_dtor@@YAXUSmallWithDtor@@@Z"(<{ %struct.SmallWithDtor }>* inalloca) {{.*}} {
 // WIN32:   call x86_thiscallcc void @"??1SmallWithDtor@@QAE@XZ"
 // WIN32: }
 // WIN64: define dso_local void @"?small_arg_with_dtor@@YAXUSmallWithDtor@@@Z"(i32 %s.coerce) {{.*}} {
 // WIN64:   call void @"??1SmallWithDtor@@QEAA@XZ"
 // WIN64: }
 // WOA64: define dso_local void @"?small_arg_with_dtor@@YAXUSmallWithDtor@@@Z"(i64 %s.coerce) {{.*}} {
-// WOA64:   call void @"??1SmallWithDtor@@QEAA@XZ"
+// WOA64:   call void @"??1SmallWithDtor@@QEAA@XZ"(%struct.SmallWithDtor* %s)
 // WOA64: }
 
 // FIXME: MSVC incompatible!
 // WOA: define dso_local arm_aapcs_vfpcc void @"?small_arg_with_dtor@@YAXUSmallWithDtor@@@Z"(%struct.SmallWithDtor* %s) {{.*}} {
 // WOA:   call arm_aapcs_vfpcc void @"??1SmallWithDtor@@QAA@XZ"(%struct.SmallWithDtor* %s)
 // WOA: }
 
+
+// Test that the eligible non-aggregate is passed directly, but returned
+// indirectly on ARM64 Windows.
+// WOA64: define dso_local void @"?small_arg_with_private_member@@YA?AUSmallWithPrivate@@U1@@Z"(%struct.SmallWithPrivate* inreg noalias sret %agg.result, i64 %s.coerce) {{.*}} {
+SmallWithPrivate small_arg_with_private_member(SmallWithPrivate s) { return s; }
+
 void call_small_arg_with_dtor() {
   small_arg_with_dtor(SmallWithDtor());
 }
 // WIN64-LABEL: define dso_local void @"?call_small_arg_with_dtor@@YAXXZ"()
 // WIN64:   call %struct.SmallWithDtor* @"??0SmallWithDtor@@QEAA@XZ"
 // WIN64:   call void @"?small_arg_with_dtor@@YAXUSmallWithDtor@@@Z"(i32 %{{.*}})
 // WIN64:   ret void