[libc++] Annotate the data member of variant with no_unique_address #137783
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Alex Aktsipetrov (gizmondo) ChangesThis allows clients to reuse tail padding after the index (if any), by applying no_unique_address attribute to their variant fields. E.g. the following struct would have smaller size. Currently no_unique_address in this code doesn't do anything, so this is an ABI break. Full diff: https://github.com/llvm/llvm-project/pull/137783.diff 3 Files Affected:
diff --git a/libcxx/include/__configuration/abi.h b/libcxx/include/__configuration/abi.h
index 01a4a4c023983..f867fb12206a7 100644
--- a/libcxx/include/__configuration/abi.h
+++ b/libcxx/include/__configuration/abi.h
@@ -57,6 +57,8 @@
// Use the smallest possible integer type to represent the index of the variant.
// Previously libc++ used "unsigned int" exclusively.
# define _LIBCPP_ABI_VARIANT_INDEX_TYPE_OPTIMIZATION
+// Allow to reuse tail padding after the index of the variant with [[no_unique_address]] attribute.
+# define _LIBCPP_ABI_VARIANT_NO_UNIQUE_ADDRESS_OPTIMIZATION
// Unstable attempt to provide a more optimized std::function
# define _LIBCPP_ABI_OPTIMIZED_FUNCTION
// All the regex constants must be distinct and nonzero.
diff --git a/libcxx/include/variant b/libcxx/include/variant
index 74a464d27ead4..b4f0856ebb17f 100644
--- a/libcxx/include/variant
+++ b/libcxx/include/variant
@@ -1319,7 +1319,11 @@ public:
# endif
private:
- __variant_detail::__impl<_Types...> __impl_;
+# ifdef _LIBCPP_ABI_VARIANT_NO_UNIQUE_ADDRESS_OPTIMIZATION
+ _LIBCPP_NO_UNIQUE_ADDRESS
+# endif // _LIBCPP_ABI_VARIANT_NO_UNIQUE_ADDRESS_OPTIMIZATION
+ __variant_detail::__impl<_Types...>
+ __impl_;
friend struct __variant_detail::__access::__variant;
friend struct __variant_detail::__visitation::__variant;
diff --git a/libcxx/test/libcxx/utilities/variant/variant.variant/variant_size.pass.cpp b/libcxx/test/libcxx/utilities/variant/variant.variant/variant_size.pass.cpp
index 2f1ea8bffb479..62abcd364c7ac 100644
--- a/libcxx/test/libcxx/utilities/variant/variant.variant/variant_size.pass.cpp
+++ b/libcxx/test/libcxx/utilities/variant/variant.variant/variant_size.pass.cpp
@@ -70,6 +70,22 @@ struct type_with_index {
#endif
};
+struct alignas(16) A16 {};
+struct VariantWithNoUniqueAddress {
+ TEST_NO_UNIQUE_ADDRESS std::variant<A16> a;
+ bool b;
+};
+struct VariantWithoutNoUniqueAddress {
+ std::variant<A16> a;
+ bool b;
+};
+constexpr bool ExpectSmallerSizeWithNoUniqueAddress =
+#ifdef _LIBCPP_ABI_VARIANT_NO_UNIQUE_ADDRESS_OPTIMIZATION
+ true;
+#else
+ false;
+#endif
+
int main(int, char**) {
test_index_type<unsigned char>();
// This won't compile due to template depth issues.
@@ -84,5 +100,8 @@ int main(int, char**) {
static_assert(sizeof(std::variant<char, int, long>) == sizeof(type_with_index<long>));
static_assert(sizeof(std::variant<std::size_t, std::size_t, std::size_t>) == sizeof(type_with_index<std::size_t>));
+ static_assert((sizeof(VariantWithNoUniqueAddress) < sizeof(VariantWithoutNoUniqueAddress)) ==
+ ExpectSmallerSizeWithNoUniqueAddress);
+
return 0;
}
|
libcxx/test/libcxx/utilities/variant/variant.variant/variant_size.pass.cpp
Show resolved
Hide resolved
…ttribute. This allows clients to reuse tail padding after the index (if any), by applying [[no_unique_address]] to variant fields.
There was a problem hiding this comment.
I am not sure if you are aware of the bug that we had in std::expected, where we applied no_unique_address to the union. The bug was that, every time we emplace, the construct_at will zero initialise the tail padding of the union, erasing anything that lives in the tail padding of the union of T or Err, including the discriminator and any other user data if the user chose to no_unique_address of the whole expected.
We ended up have to ABI break to fix expected. (only conditionally apply no_unique_address and do not allow user's type to fit into's union's tail padding). We only allow the user data to reuse padding when the discriminator is not in the tail padding of the union (meaning the emplace will not touch the user data)
I think this variant case is quite similar and just wanted to double check if you have thought about this.
so to test this case, we want something that is big enough to fit our index type into the tail padding of T
struct F {
F(){} // to make it not a c-struct
long double c{};
bool b{};
};
struct S {
[[no_unique_address]] std::variant<int, F> v;
bool b;
}
S s{};
s.b = true;
s.v.emplace<F>(......);
assert(s.b)
so in summary, i think to have an optimal solution, we want the layout to be something like this struct repr {
[[no_unique_address]] the_recursive_union<...> t;
index_t index;
};
// If index can fit into the tail padding of the union of variant, we want index to be inside the tail padding, but not the user data
class variant {
repr repr_;
};
// if index cannot fit into the tail padding, we can allow user to reuse the tail padding after the index, because it is not going to be overwritten
class variant {
[[no_unique_address]] repr repr_;
};
|
Yeah I was aware of this issue. The current version of the PR is safe because the variant at the moment does not try to put the index in the tail padding of the user types. So what you're saying is that it should, and this ought to be implemented together with (conditionally) allowing users to reuse tail padding? The idea behind bundling it together is to not expand the zoo of potential variant layouts in the future, right? |
Exactly. If this PR does not do that, I think we should, as we are creating a new ABI mode anyway |
Makes sense. I'll try to take a stab at this. Converting to draft for now. @huixie90, I'm curious - does your suggestion mean there is an appetite to make the same layout optimization for std::optional? I'd expect it to be more impactful there. |
yes I agree. |
This allows clients to reuse tail padding after the index (if any), by applying no_unique_address attribute to their variant fields.
E.g. the following struct would have smaller size.
Currently no_unique_address in this code doesn't do anything, so this is an ABI break.