[PATCH] D60349: [COFF, ARM64] Fix ABI implementation of struct returns

[Richard Townsend (Arm) via Phabricator via cfe-commits]

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