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

Tue Apr 30 15:01:07 PDT 2019

efriedma added inline comments.

================
Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1106
+
+    FI.getReturnInfo().setInReg(isAArch64 && !IsSizeGreaterThan128(RD));

----------------
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.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60349/new/

https://reviews.llvm.org/D60349