[PATCH] D89362: [MS] Apply `inreg` to AArch64 sret parms on instance methods
Reid Kleckner via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Oct 14 09:41:25 PDT 2020
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
More information about the cfe-commits
mailing list