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

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

rsmith added a comment.

In D114732#3159247 <https://reviews.llvm.org/D114732#3159247>, @Quuxplusone wrote:

> - 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.

I've compared the two patches, and as far as I can tell, this patch is effectively the subset of D50119 <https://reviews.llvm.org/D50119> that includes only the new type trait and not the new attributes, with the following exceptions:

- No manual `__has_extension` support. This is an improvement in this patch; the intended way to test for type trait primitives is with `__has_builtin`, and that has built-in knowledge of type traits. I think this may have changed since D50119 <https://reviews.llvm.org/D50119> was first proposed; `__has_builtin` has not always supported type trait builtins.
- This patch has an unnecessary check for trivial destruction of trivially copyable types (commented in this review).
- This patch treats trivial-for-calls as implying trivially-relocatable, and in particular this means that `[[trivial_abi]]` types are implicitly trivially-relocatable, as discussed below.
- `-ast-dump` support. I'd be inclined to omit that from this change; until / unless we have an attribute that makes a class trivially-relocatable but not trivial for calls, adding dump support for `canPassInRegisters` rather than `isTriviallyRelocatable` seems better-motivated, but wouldn't make sense as part of this patch.
- Different set of test cases.

Even if we want to land all of the functionality in D50119 <https://reviews.llvm.org/D50119> now, splitting out the new type trait and the new attribute into separate patches makes sense to me.

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

I agree with @rjmccall on this: `[[trivial_abi]]` is meant to imply both that it's correct to trivially relocate and that we should perform a trivial relocation when making a function call. The latter without the former doesn't make sense to me. I agree that the former without the latter does make sense, and I don't think this change is an impediment to supporting that.

Personally I'm more comfortable landing this subset of D50119 <https://reviews.llvm.org/D50119> now than I would be with landing all of D50119 <https://reviews.llvm.org/D50119>, given that the type trait seems well-motivated and unlikely to have any serious conflicts with an eventually-standardized feature. John's concern that the semantics of an eventual `[[trivially_relocatable]]` attribute may differ from what's in D50119 <https://reviews.llvm.org/D50119> seem well-founded to me.



================
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.
----------------
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.


================
Comment at: clang/lib/AST/Type.cpp:2500
+    return BaseElementType.isTriviallyCopyableType(Context) &&
+           !BaseElementType.isDestructedType();
+  }
----------------
You can simplify this a little by dropping this `isDestructedType()` check; trivially copyable implies trivially destructible.

It would be a little more precise to check for `!isNonTrivialToPrimitiveDestructiveMove() && !isDestructedType()` here; that'd treat `__autoreleasing` pointers as trivially-relocatable, but not `__strong` pointers (because "destructive move" there actually means "non-destructive move" and needs to leave the object in a valid but unspecified state, which means nulling out a moved-from `__strong` pointer). I think getting this "fully" right would require a bit more plumbing to properly handle the case of a `struct` containing a `__strong` pointer (which is trivially relocatable but not trivially anything else).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114732