Quuxplusone added a comment.

On the one hand, I like that someone else is pushing a patch that's basically 
the same as D50119 <https://reviews.llvm.org/D50119>, because maybe if enough 
people reimplement the same thing, eventually Clang will accept one of them. ;)
However, I'd like to point out:

- D50119 <https://reviews.llvm.org/D50119> has more extensive tests, and has 
been live on https://p1144.godbolt.org/z/axx9Wj3r5 for a couple years now; if 
the consensus is that Clang wants "part of D50119 
<https://reviews.llvm.org/D50119> but not all of it," then IMHO it would be 
reasonable to put some comments on D50119 <https://reviews.llvm.org/D50119> 
with the goal of stripping it //down//, rather than trying to build //up// a 
smaller version of it in parallel.
- (Major point of contention) To me, `[[trivial_abi]]` does not imply 
`[[trivially_relocatable]]` at all. I believe @rjmccall disagreed a few years 
ago: basically I said "The two attributes are orthogonal," with practical 
demonstration, and basically he said "yes, in practice you're right, but 
philosophically that's a bug and we do actually //intend// `[[trivial_abi]]` to 
imply `[[trivially_relocatable]]` even though the compiler doesn't enforce it" 
(but then AFAIK nothing was ever done about that).

Here are my practical examples: https://p1144.godbolt.org/z/EYcMM1nTW
With `ATTR=[[clang::trivial_abi]]`, we see that `take(g)` passes in a register; 
but notice that it still calls the copy-ctor, and it will still call the dtor 
(inside `take` — the ABI changes to "callee-destroy" for arguments of these 
types). And we see that `std::swap` is completely unaffected: we know that the 
calling convention for passing `T` by value is altered, but we don't know 
anything //semantic// about the behavior of `T` itself (e.g. when it is 
relocated or swapped).
With `ATTR=[[clang::trivially_relocatable]]` (D50119 
<https://reviews.llvm.org/D50119>), we see that `take(g)` still passes on the 
stack (p1144 trivial relocation is backward-compatible with C++20 by design: it 
doesn't alter the calling convention at all). But notice that because we know 
something //semantic// about the behavior of `T` (namely, that it is trivially 
relocatable), `std::swap` is able to skip calling its ctors and dtors and just 
swap the bits directly.

Therefore, I support something close to this patch, but I strongly believe that 
you need to split up the "trivially relocatable" attribute itself from "trivial 
abi". They are represented orthogonally in the AST, and they should be 
toggleable orthogonally by the end-user. The end-user might want a trivial-abi 
type that is not trivially-relocatable, or a trivially-relocatable type that is 
not trivial-abi.

Related reading: https://quuxplusone.github.io/blog/2018/05/02/trivial-abi-101/
Related watching, on p1144 `[[trivially_relocatable]]` and adjacent topics such 
as `trivial_abi` and `move_relocates`: 
https://www.youtube.com/watch?v=SGdfPextuAU


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114732

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to