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