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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to