zoecarver added a comment. In D92361#2459655 <https://reviews.llvm.org/D92361#2459655>, @rjmccall wrote:
> In D92361#2459513 <https://reviews.llvm.org/D92361#2459513>, @zoecarver wrote: > >>> I think that as long as the class leaves a copy/move constructor defaulted, >>> there's no need for a new trivial_abi attribute. >> >> Sorry, I'm not sure I follow. Could you elaborate a bit or provide an >> example? What do you mean by "new" trivial_abi attribute? > > Sorry, I mean that I think Akira's example should be passed directly. It > shouldn't require its own trivial_abi attribute in order to get the treatment. No worries. I understand now. The problem we're discussing actually has nothing to do with the trivial-abi attribute. We just need to make sure that non-trival-abi types are not affected by this change (and it appears they were). ================ 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()) ---------------- Quuxplusone wrote: > 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? :) > 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. IIUC (and that's a big "if") being trivial for the purposes of a call, does not mean it will _always_ be passed through registers. (For example, we'd never pass a 512-byte type through registers.) And, I don't think they do have the same calling convention, though they are both passed indirectly. As Akira rightly pointed out, on ToT `D` has the `byval` attribute indicating that it _could_ be passed directly (according to the ABI/standard at least). While it might not be an observable change in the assembly, my patch (or at least the current version on phab) removes the byval argument. I have a local version that fixes this, though, and I'll upload that shortly. 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
