[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:3211-3215
+The compiler will pass and return a trivially relocatable type using the C ABI
+for the underlying type, even when the type would otherwise be considered
+non-trivially-relocatable. If a type is trivially relocatable, has a
+non-trivial destructor, and is passed as an argument by value, the convention
+is that the callee will destroy the object before returning.
----------------
Quuxplusone wrote:
> devin.jeanpierre wrote:
> > rsmith wrote:
> > > I think this documentation change has mixed together two different things. The revised wording says that `[[trivial_abi]]` implies trivially-relocatable and that trivially-relocatable implies passing using the C ABI, which is wrong: the second implication does not hold. What we should say is that `[[trivial_abi]]` (if we're not in the "has no effect" case described below) implies both that the type is trivially-relocatable, and that it is passed using the C ABI (that is, that we trivially relocate in argument passing).
> > > 
> > > Instead of the wording changes you have here, perhaps we could leave the old wording alone and add a paragraph that says that a type with the attribute is assumed to be trivially-relocatable for the purpose of `__is_trivially_relocatable`, but that Clang doesn't yet take advantage of this fact anywhere other than argument passing.
> > Yeah, this was too aggressive a wording change, and might easily change later when `[[trivially_relocatable]]` is added.
> > 
> > I did drop the "Clang doesn't yet take advantage of this fact anywhere other than argument passing.", because I feel like -- does that sort of sentiment belong instead in the docs for `__is_trivially_relocatable`? Maybe I should change `__is_trivially_relocatable` to note that this is never taken advantage of anywhere except in by-value argument passing (when the type is trivial for the purpose of calls) and library code.
> Richard's comment jogged my brain and made we worry about this again. IIUC, you're saying:
> 
> 1. All trivial-abi types //are in fact// trivially relocated, specifically when they are passed to functions.
> 2. Therefore, all trivial-abi types are "trivially relocatable" //in general//.
> 3. Therefore, all trivial-abi types are safe to optimize in all the ways depicted in D67524.
> 
> I agree with 1 for sure, but I'm skeptical of the logical soundness of 2 and 3. Are we willing to set it in stone?
> 
> If we're //not// willing to set that whole syllogism in stone, then I would object to this patch. Because of //this// syllogism:
> 
> 4. This patch makes Clang's `__is_trivially_relocatable(T)` return `true` for all trivial-abi types.
> 5. D67524 (rightly) applies its optimizations to all types for which `__is_trivially_relocatable(T)` is `true`.
> 6. Therefore, if we adopt this patch, we'd better be dang sure that all trivial-abi types are safe to optimize in all the ways depicted in D67524.
> 
> That is, we must set in stone the idea that all trivial-abi types can safely:
> - be swapped trivially, in terms of bytewise swap, inside e.g. std::sort
> - use trivial (bytewise) relocation inside vector reallocation
> - use trivial (bytewise) relocation when shifting inside vector::insert or vector::erase
> 
> If @devin.jeanpierre and @rsmith and @rjmccall and @ahatanak are all nodding along to this and saying "yeah, totally, every premise above is true and the syllogisms are all valid and those optimizations are all valid, for every trivial-abi type, forever," then I'm happy.
What use cases would we be giving up on if we go this way? What would a type look like for which trivial relocation when passing to functions is correct, but for which trivial relocation in general is either incorrect or undesirable?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114732