[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

[Zoe Carver via Phabricator via cfe-commits]

zoecarver 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())
----------------
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92361