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

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.


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


================
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();
----------------
efriedma wrote:
> 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?
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89362