[PATCH] D60349: [COFF, ARM64] Fix ABI implementation of struct returns
richard.townsend.arm added a comment. Just completed testing... everything seems to be working correctly. So long as the all the tests pass, let's commit! :D CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60349/new/ https://reviews.llvm.org/D60349 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60349: [COFF, ARM64] Fix ABI implementation of struct returns
richard.townsend.arm added a comment. And with those modifications, everything seems to work correctly. I'd be fine with the LLVM portion landing at this stage, but one more rev and another test cycle for this patch would be ideal. Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1085 +return true; + if (IsSizeGreaterThan128(RD)) +return true; So... to get the latest diff working, I had to remove this check... Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1096 + bool isAArch64 = CGM.getTarget().getTriple().isAArch64(); + bool isIndirectReturn = isAArch64 ? +(passClassIndirect(RD) || hasMicrosoftABIRestrictions(RD)) : I also had to amend this to: ```bool isIndirectReturn = isAArch64 ? (passClassIndirect(RD) || hasMicrosoftABIRestrictions(RD) || IsSizeGreaterThan128(RD)) : !RD->isPOD();``` Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1106 + +FI.getReturnInfo().setInReg(isAArch64 && !IsSizeGreaterThan128(RD)); ... and also amend this to `FI.getReturnInfo().setInReg(isAArch64 && !(isAggregate && IsSizeGreaterThan128(RD)));` CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60349/new/ https://reviews.llvm.org/D60349 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60349: [COFF, ARM64] Fix ABI implementation of struct returns
richard.townsend.arm added inline comments. Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1106 + +FI.getReturnInfo().setInReg(isAArch64 && !IsSizeGreaterThan128(RD)); efriedma wrote: > richard.townsend.arm wrote: > > efriedma wrote: > > > richard.townsend.arm wrote: > > > > I'm not sure what the IsSizeGreaterThan128 check is doing here - if the > > > > return type is over 128 bits, then it will be indirectly returned in X8 > > > > with this check, which is not always what we want (e.g. in > > > > https://bugs.llvm.org/show_bug.cgi?id=41135 ostream.cpp, MSVC expects > > > > the value returned in X1). Another check of hasMicrosoftABIRestrictions > > > > might be OK, but that's also not quite right due to the size check. > > > I think the check here is intended to counteract the similar check in > > > hasMicrosoftABIRestrictions... but yes, I don't think that works > > > correctly. Looks like we're missing a testcase for a non-aggregate type > > > with size greater than 128 bits. > > Ah, OK. I think the problem is that we need to check whether the return > > value meets the rest of aggregate definition (i.e. > > !hasMicrosoftABIRestrictions), as well as being more than 128 bits in size. > > May be worth splitting the logic up. Will experiment. > I think you might get the right behavior by just erasing both > IsSizeGreaterThan128 checks. That matches the way the ABI document describes > the rules. Haven't tried it, though. So I tried that, and everything mostly worked, but there are still some crashes in electron. I think the solution will be something like: 1. Remove the size restriction from the hasMicrosoftABIRestrictions and just decide if the result is aggregate/not aggregate according to the ABI spec. 2. Replace the check with !(isAggregate && IsSizeGreaterThan128(RD)) If both of those things are are true (i.e. it's an aggregate more than >128bits) we should end up calling FI.getReturnInfo().setInReg(false), which implies a return in x8. I haven't confirmed that this works yet because I've run out of time today, but the result should be in tomorrow. May need similar logic on the outer if. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60349/new/ https://reviews.llvm.org/D60349 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60349: [COFF, ARM64] Fix ABI implementation of struct returns
richard.townsend.arm added inline comments. Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1106 + +FI.getReturnInfo().setInReg(isAArch64 && !IsSizeGreaterThan128(RD)); efriedma wrote: > richard.townsend.arm wrote: > > I'm not sure what the IsSizeGreaterThan128 check is doing here - if the > > return type is over 128 bits, then it will be indirectly returned in X8 > > with this check, which is not always what we want (e.g. in > > https://bugs.llvm.org/show_bug.cgi?id=41135 ostream.cpp, MSVC expects the > > value returned in X1). Another check of hasMicrosoftABIRestrictions might > > be OK, but that's also not quite right due to the size check. > I think the check here is intended to counteract the similar check in > hasMicrosoftABIRestrictions... but yes, I don't think that works correctly. > Looks like we're missing a testcase for a non-aggregate type with size > greater than 128 bits. Ah, OK. I think the problem is that we need to check whether the return value meets the rest of aggregate definition (i.e. !hasMicrosoftABIRestrictions), as well as being more than 128 bits in size. May be worth splitting the logic up. Will experiment. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60349/new/ https://reviews.llvm.org/D60349 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60349: [COFF, ARM64] Fix ABI implementation of struct returns
richard.townsend.arm added inline comments. Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1106 + +FI.getReturnInfo().setInReg(isAArch64 && !IsSizeGreaterThan128(RD)); I'm not sure what the IsSizeGreaterThan128 check is doing here - if the return type is over 128 bits, then it will be indirectly returned in X8 with this check, which is not always what we want (e.g. in https://bugs.llvm.org/show_bug.cgi?id=41135 ostream.cpp, MSVC expects the value returned in X1). Another check of hasMicrosoftABIRestrictions might be OK, but that's also not quite right due to the size check. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60349/new/ https://reviews.llvm.org/D60349 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60349: [COFF, ARM64] Fix ABI implementation of struct returns
richard.townsend.arm added inline comments.
Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1080
+ if (const auto *Constructor = dyn_cast(RD))
+if (Constructor->isUserProvided())
+ return true;
So I think that the problem with this new check is that it doesn't check all of
the constructors. I replaced it with
for (auto it = RD->ctor_begin(); it != RD->ctor_end(); ++it) {
if (it->isUserProvided())
return true;
}
And that seems to resolve the `setw` problem.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60349/new/
https://reviews.llvm.org/D60349
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60349: [COFF, ARM64] Fix ABI implementation of struct returns
richard.townsend.arm added a comment. Apologies, I meant to write "here's what Clang is trying to call" in the previous comment. It's clear from looking at MSVC's output that MSVC expects to return indirectly via X0, implying that `_Smanip` is not aggregate by MSVC's definition. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60349/new/ https://reviews.llvm.org/D60349 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60349: [COFF, ARM64] Fix ABI implementation of struct returns
richard.townsend.arm added a comment.
The current diff (196894) seems to have the same `std::setw` issue as the
previous one. Here's what it's trying to compile:
_MRTIMP2 _Smanip __cdecl setw(streamsize wide)
{ // manipulator to set width
return (_Smanip(, wide));
}
Here's the definition for `_Smanip`:
template
struct _Smanip
{ // store function pointer and argument value
_Smanip(void (__cdecl *_Left)(ios_base&, _Arg), _Arg _Val)
: _Pfun(_Left), _Manarg(_Val)
{ // construct from function pointer and argument value
}
void (__cdecl *_Pfun)(ios_base&, _Arg);// the function
pointer
_Arg _Manarg; // the argument value
};
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60349/new/
https://reviews.llvm.org/D60349
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60349: [COFF, ARM64] Fix ABI implementation of struct returns
richard.townsend.arm added a comment. Confirmed just now that the current diff (196774) does work, but it'd be good to correct user-provided constructors and trivial destructors before this lands. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60349/new/ https://reviews.llvm.org/D60349 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60349: [COFF, ARM64] Fix ABI implementation of struct returns
richard.townsend.arm added inline comments. Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1074 + if (!RD->hasTrivialCopyAssignment()) +return true; + return false; richard.townsend.arm wrote: > Should this function also check for user-provided constructors? I think it should: I speculatively added these two lines if (RD->hasUserDeclaredConstructor()) return true; and it resolved the problem with `std::setw` I mentioned in the bug tracker (which means Electron could start). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60349/new/ https://reviews.llvm.org/D60349 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60349: [COFF, ARM64] Fix ABI implementation of struct returns
richard.townsend.arm added inline comments. Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1074 + if (!RD->hasTrivialCopyAssignment()) +return true; + return false; Should this function also check for user-provided constructors? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60349/new/ https://reviews.llvm.org/D60349 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60349: [COFF, ARM64] Fix ABI implementation of struct returns
richard.townsend.arm added inline comments.
Comment at: lib/Sema/SemaDeclCXX.cpp:5908
+// The checks below ensure that thare no user provided constructors.
+// For AArch64, we use the C++14 definition of an aggregate, so we also
+// check for:
I think these checks are in the wrong place - the aggregate restriction applies
to return values, but it doesn't apply to arguments. Placing this logic here
breaks argument passing between MSVC and Clang, because Clang (with this logic
in place) will pass and expect arguments with an extra layer of indirection:
for example, when passing
https://cs.chromium.org/chromium/src/v8/include/v8.h?q=v8::Local=package:chromium=0=183
v8::Local, what you actually see in the register bank is supposed to be its
only private member. Moving the logic to MicrosoftCXXABI and applying it only
to the return value resolves this problem.
Comment at: lib/Sema/SemaDeclCXX.cpp:5916
+if (isAArch64) {
+ if (D->getAccess() == AS_private || D->getAccess() == AS_protected)
+return false;
efriedma wrote:
> D->getAccess() is the access specifier for the class itself (if the class is
> nested inside another class), not the access specifier for any of its members.
>
> I think you're looking for HasProtectedFields and HasPrivateFields. (I think
> there currently isn't any getter on CXXRecordDecl for them at the moment, but
> you can add one.)
So the issue that I mentioned in
https://bugs.llvm.org/show_bug.cgi?id=41135#c18 is resolved by checking
HasProtectedFields/HasPrivateFields.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60349/new/
https://reviews.llvm.org/D60349
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60349: [COFF, ARM64] Fix ABI implementation of struct returns
richard.townsend.arm added inline comments. Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1090 +FI.getReturnInfo().setSRetAfterThis(isInstanceMethod); +FI.getReturnInfo().setInReg(isAArch64 && isIndirectReturn); Based on https://reviews.llvm.org/D60348 - lib/Target/AArch64/AArch64CallingConvention.td (line 44), should this be (isIndirectReturn || isInstanceMethod)? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60349/new/ https://reviews.llvm.org/D60349 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60349: [COFF, ARM64] Fix ABI implementation of struct returns
richard.townsend.arm added inline comments. Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1058 + + // 1. For return types <= 16 bytes, use the C return semantics. + Microsoft have updated the spec since this was written, it now says to check for aggregate-ness, trivial copy, and trivial destruct, instead of POD. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60349/new/ https://reviews.llvm.org/D60349 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
