Quuxplusone added inline comments.
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:6502
+ // except that it has a non-trivial member *with* the trivial_abi attribute.
+ for (auto Base : D->bases()) {
+ if (auto CxxRecord = Base.getType()->getAsCXXRecordDecl())
----------------
zoecarver wrote:
> ahatanak wrote:
> > zoecarver wrote:
> > > ahatanak wrote:
> > > > It looks like this patch changes the way `D` is passed in the following
> > > > code:
> > > >
> > > > ```
> > > > struct B {
> > > > int i[4];
> > > > B();
> > > > B(const B &) = default;
> > > > B(B &&);
> > > > };
> > > >
> > > > struct D : B {
> > > > D();
> > > > D(const D &) = default;
> > > > D(D &&) = delete;
> > > > };
> > > >
> > > > void testB(B a);
> > > > void testD(D a);
> > > >
> > > > void testCallB() {
> > > > B b;
> > > > testB(b);
> > > > }
> > > >
> > > > void testCallD() {
> > > > D d;
> > > > testD(d);
> > > > }
> > > > ```
> > > >
> > > > `B` cannot be passed in registers because it has a non-trivial move
> > > > constructor, whereas `D` can be passed in registers because the move
> > > > constructor is deleted and the copy constructor is trivial.
> > > >
> > > > I'm not sure what the best way to handle this is, but I just wanted to
> > > > point this out.
> > > Hmm. Good catch. One way to fix this would be to simply create a
> > > `HasPassableSubobject` variable and add that to the conditions below
> > > (instead of returning false here). But, it seems that `D` isn't passed by
> > > registers (even though, maybe it should be) on ToT:
> > > https://godbolt.org/z/4xevW5
> > >
> > > Given that, do you think it's OK to return false here, or should I update
> > > this patch to use the logic I just described (even though that would be a
> > > nfc)?
> > The argument is byval, so `D` is passed directly. If you remove `-O3` and
> > add `-target aarch64`, you'll see that `[2 x i64]` is being passed
> Ah, I see now. Great. Thanks. I'll update the patch.
Akira's example is legal C++ with no Clang-specific attributes, so its behavior
is governed by the appropriate platform's ABI doc — there exists one correct
answer.
At least on x86-64 with the Itanium ABI, GCC and ICC and Clang ToT all agree on
the answer: `B` and `D` have exactly the same passing convention. If your patch
breaks that, that's a problem.
Contrariwise, it appears that `B` and `D` have different passing conventions on
"armv8" according to Clang, and the same passing convention on "ARM64"
according to GCC: https://godbolt.org/z/j9jzYG
Of course if the programmer adds `[[clang::trivial_abi]]` to one of them, then
all bets are off, standards-wise, and you're free to figure out a way to pass
it in registers if you want to. But otherwise I think you have to follow what
the ABI says.
Bear in mind that I don't really know ABIs other than Itanium/x86-64. Maybe the
problem here is that other platforms don't have well-defined ABIs and so we get
to make one up? Maybe everyone except me is already aware that that's what
we're doing? :)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92361/new/
https://reviews.llvm.org/D92361
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
