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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.


================
Comment at: clang/lib/CodeGen/MicrosoftCXXABI.cpp:1077
 
-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
----------------
This is just reversing the result of the function, I guess.


================
Comment at: clang/lib/CodeGen/MicrosoftCXXABI.cpp:1108
+  bool isTrivialForABI =
+      RD->canPassInRegisters() && !(isAArch64 && !isCXX14Aggregate(RD));
   bool isIndirectReturn =
----------------
isTrivialForABI is only used if isAArch64 is true, so it shouldn't use isTrivialForABI as part of its computation, I think?


================
Comment at: clang/lib/CodeGen/MicrosoftCXXABI.cpp:1110
   bool isIndirectReturn =
-      isAArch64 ? (!RD->canPassInRegisters() ||
-                   IsSizeGreaterThan128(RD))
-                : !RD->isPOD();
+      isAArch64 ? (!isTrivialForABI || IsSizeGreaterThan128(RD)) : !RD->isPOD();
   bool isInstanceMethod = FI.isInstanceMethod();
----------------
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89362/new/

https://reviews.llvm.org/D89362