[PATCH] D133817: MSVC ABI: Looks like even non-aarch64 uses the MSVC/14 definition for pod/aggregate passing

Thu Sep 15 23:35:59 PDT 2022

dblaikie added a subscriber: majnemer.
dblaikie added a comment.

In D133817#3790057 <https://reviews.llvm.org/D133817#3790057>, @rnk wrote:

> Thanks!
>
>> But I had
>> trouble figuring out how to exercise this functionality properly to add
>> test coverage and then compare that to MSVC itself... - I got very
>> confused/turned around trying to test this, so I've given up enough to
>> send what I have out for review, but happy to look further into this
>> with help.
>
> I think I would take the three cases you found (protected, nsdmi, defaulted copy ctor), add each to this file, and check for the IR prototypes here in this test file. Either sret is used, or it is not.

OK, done that. Removed the other attempt I'd made at the protected-field-in-field test case in favor of the one at the end with the other two cases, since that seemed clearer.

> Regarding HFAs, I believe that logic is only used on x86/x64 for the __vectorcall calling convention.

Oooh.. hmm, that should help. I'll look around.

> I believe it is well-tested with respect to C-like structs, but the C++ aspects that you are changing are not well tested. I think I managed to construct a case using NSDMIs (Richard used to prefer the terminology "default member initializers" which is simpler): https://godbolt.org/z/daPzKxj3h It looks like we don't match arm64 MSVC's behavior exactly here, but your change to remove the "!isAArch64" test would probably cause us to change behavior in this case, right?

It would change the behavior, but I think in the wrong direction... making non-AArch64 behave like the buggy AArch64... hrm.

So I think https://godbolt.org/z/Md91eE8KM demonstrates that the homogenous aggregate rules are different for AArch64 from x86_64 - so we aren't going to/shouldn't lose the AArch64 check in `isPermittedToBeHomogeneousAggregate`.

But it's also true that the rules are different again from the `isTrivialForMSVC` - as per your test with "default member initializers", this doesn't seem to change the return ABI, but it does make the (only the AArch64, not the x86_64) consider the type non-homogenous-aggregate.

(even though the ways that homogenous aggregate rules are inconsistent with `isTrivialForMSVC`, `isTrivialForMSVC` does agree between x86 and AArch64... so it suggests that generalizing that function across x86 and AArch64 is correct, but if someone wants to fix the AArch64 vectorcall ABI, they will need to do so by introducing a separate function, `isHomogenousAggregateForAArch64MSVC` or the like)

So, I could leave a test case and some comments, or follow-up with a separate fix for the homogenous aggregate issue? I guess it comes down to writing that separate function to do the tests. (would be great to throw some of this at the ABI fuzzer @majnemer had back in the day, at least I think he had an ABI fuzzer of some sort, yeah?)

================
Comment at: clang/test/CodeGenCXX/microsoft-abi-sret-and-byval.cpp:77

+struct SmallWithSmallWithPrivate {
+  SmallWithPrivate p;
----------------
rnk wrote:
> I'm amused that this class is passed directly, but if you pass the field by itself, it is passed indirectly. :shrug:
> 
Yeah, isn't that... neat?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133817