[libc++] Make sure that __desugars_to isn't tripped up by reference_wrapper, const and ref qualifiers #132092
Conversation
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Louis Dionne (ldionne) ChangesPreviously, any cv-ref qualification on an operation would cause __desugars_to to report false, which would lead to unnecessary pessimizations. The same holds for reference_wrapper. In practice, cv-ref qualifications on the operation itself are not relevant to determining whether an operation desugars to something else or not. Fixes #129312 Full diff: https://github.com/llvm/llvm-project/pull/132092.diff 4 Files Affected:
diff --git a/libcxx/include/__functional/reference_wrapper.h b/libcxx/include/__functional/reference_wrapper.h
index d6cd6428f22db..3678b5553c444 100644
--- a/libcxx/include/__functional/reference_wrapper.h
+++ b/libcxx/include/__functional/reference_wrapper.h
@@ -15,6 +15,7 @@
#include <__config>
#include <__functional/weak_result_type.h>
#include <__memory/addressof.h>
+#include <__type_traits/desugars_to.h>
#include <__type_traits/enable_if.h>
#include <__type_traits/invoke.h>
#include <__type_traits/is_const.h>
@@ -149,6 +150,11 @@ void ref(const _Tp&&) = delete;
template <class _Tp>
void cref(const _Tp&&) = delete;
+// Let desugars-to pass through std::reference_wrapper
+template <class _CanonicalTag, class _Operation, class... _Args>
+inline const bool __desugars_to_v<_CanonicalTag, reference_wrapper<_Operation>, _Args...> =
+ __desugars_to_v<_CanonicalTag, _Operation, _Args...>;
+
_LIBCPP_END_NAMESPACE_STD
#endif // _LIBCPP___FUNCTIONAL_REFERENCE_WRAPPER_H
diff --git a/libcxx/include/__type_traits/desugars_to.h b/libcxx/include/__type_traits/desugars_to.h
index 452c70bfbad66..ed8b5ab8d318a 100644
--- a/libcxx/include/__type_traits/desugars_to.h
+++ b/libcxx/include/__type_traits/desugars_to.h
@@ -52,6 +52,24 @@ struct __totally_ordered_less_tag {};
template <class _CanonicalTag, class _Operation, class... _Args>
inline const bool __desugars_to_v = false;
+// For the purpose of determining whether something desugars to something else,
+// we disregard the cv-refs of the operation itself.
+template <class _CanonicalTag, class _Operation, class... _Args>
+inline const bool __desugars_to_v<_CanonicalTag, _Operation const, _Args...> =
+ __desugars_to_v<_CanonicalTag, _Operation, _Args...>;
+template <class _CanonicalTag, class _Operation, class... _Args>
+inline const bool __desugars_to_v<_CanonicalTag, _Operation volatile, _Args...> =
+ __desugars_to_v<_CanonicalTag, _Operation, _Args...>;
+template <class _CanonicalTag, class _Operation, class... _Args>
+inline const bool __desugars_to_v<_CanonicalTag, _Operation const volatile, _Args...> =
+ __desugars_to_v<_CanonicalTag, _Operation, _Args...>;
+template <class _CanonicalTag, class _Operation, class... _Args>
+inline const bool __desugars_to_v<_CanonicalTag, _Operation&, _Args...> =
+ __desugars_to_v<_CanonicalTag, _Operation, _Args...>;
+template <class _CanonicalTag, class _Operation, class... _Args>
+inline const bool __desugars_to_v<_CanonicalTag, _Operation&&, _Args...> =
+ __desugars_to_v<_CanonicalTag, _Operation, _Args...>;
+
_LIBCPP_END_NAMESPACE_STD
#endif // _LIBCPP___TYPE_TRAITS_DESUGARS_TO_H
diff --git a/libcxx/test/libcxx/type_traits/desugars_to.compile.pass.cpp b/libcxx/test/libcxx/type_traits/desugars_to.compile.pass.cpp
new file mode 100644
index 0000000000000..780d9372d8387
--- /dev/null
+++ b/libcxx/test/libcxx/type_traits/desugars_to.compile.pass.cpp
@@ -0,0 +1,40 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include <__type_traits/desugars_to.h>
+
+struct Tag {};
+struct Operation {};
+template <>
+bool const std::__desugars_to_v<Tag, Operation> = true;
+
+void tests() {
+ // Make sure that __desugars_to is false by default
+ {
+ struct Foo {};
+ static_assert(!std::__desugars_to_v<Tag, Foo>, "");
+ }
+
+ // Make sure that __desugars_to bypasses cv and ref qualifiers on the operation
+ {
+ static_assert(std::__desugars_to_v<Tag, Operation>, ""); // no quals
+ static_assert(std::__desugars_to_v<Tag, Operation const>, "");
+ static_assert(std::__desugars_to_v<Tag, Operation volatile>, "");
+ static_assert(std::__desugars_to_v<Tag, Operation const volatile>, "");
+
+ static_assert(std::__desugars_to_v<Tag, Operation&>, "");
+ static_assert(std::__desugars_to_v<Tag, Operation const&>, "");
+ static_assert(std::__desugars_to_v<Tag, Operation volatile&>, "");
+ static_assert(std::__desugars_to_v<Tag, Operation const volatile&>, "");
+
+ static_assert(std::__desugars_to_v<Tag, Operation&&>, "");
+ static_assert(std::__desugars_to_v<Tag, Operation const&&>, "");
+ static_assert(std::__desugars_to_v<Tag, Operation volatile&&>, "");
+ static_assert(std::__desugars_to_v<Tag, Operation const volatile&&>, "");
+ }
+}
diff --git a/libcxx/test/libcxx/utilities/function.objects/refwrap/desugars_to.compile.pass.cpp b/libcxx/test/libcxx/utilities/function.objects/refwrap/desugars_to.compile.pass.cpp
new file mode 100644
index 0000000000000..30e5654c0824f
--- /dev/null
+++ b/libcxx/test/libcxx/utilities/function.objects/refwrap/desugars_to.compile.pass.cpp
@@ -0,0 +1,23 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// <functional>
+
+// reference_wrapper
+
+// Ensure that std::reference_wrapper does not inhibit optimizations based on the
+// std::__desugars_to internal helper.
+
+#include <functional>
+
+static_assert(std::__desugars_to_v<std::__equal_tag, std::equal_to<void>, int, int>,
+ "something is wrong with the test");
+
+// make sure we pass through reference_wrapper
+static_assert(std::__desugars_to_v<std::__equal_tag, std::reference_wrapper<std::equal_to<void> >, int, int>, "");
+static_assert(std::__desugars_to_v<std::__equal_tag, std::reference_wrapper<std::equal_to<void> const>, int, int>, "");
|
| @@ -52,6 +52,24 @@ struct __totally_ordered_less_tag {}; | |||
| template <class _CanonicalTag, class _Operation, class... _Args> | |||
| inline const bool __desugars_to_v = false; | |||
|
|
|||
| // For the purpose of determining whether something desugars to something else, | |||
There was a problem hiding this comment.
Do we have tests where we test this desugaring with cvref types in our algorithms?
// syntactically, the operation is equivalent to calling `a < b`, and these expressions
// have to be true for any `a` and `b`:
// - `(a < b) == (b > a)`
Here if a and b are a volatile ref to the same object c the statement above may not hold true and can change every time it's tested.
My concern is specifically about volatile.
There was a problem hiding this comment.
There is a subtlety here. This patch only disregards the cv-ref qualifiers on the operation itself, which would be std::less in this case. It does not disregard the cv-ref qualifiers on the arguments that are passed to the function.
There was a problem hiding this comment.
Still I'm concerned that a volatile qualified operator is intended not to be optimized. I expect them to be very rare so not desugaring them sounds not a big loss. IMO if we want to desugar them I'd like some tests that desugaring them has no unexpected issues in cases where a volatile operator depends on read and writes not being optimized away.
There was a problem hiding this comment.
I suspect that this is entirely theoretical at the moment, since I don't think we use volatile-qualified predicates anywhere. At least we don't desugar any such predicates that are not stateless, which is where volatile could conceivably make a difference.
I feel that __desugars_to operates at the level of "is this operation mathematically the same as that other operation", not "is this operation exactly the same as that other operation". This distinction is actually important because otherwise we might not want to also strip ref qualifiers from predicates. For the same reason, I don't think we can ever specialize __desugars_to on a predicate that is not stateless, making the volatile qualification issue somewhat moot.
Do you have something concrete in mind like an example of an algorithm that could be passed a volatile predicate and where that could be interpreted as "don't optimize this"?
In the meantime, I think stripping volatile is mostly for consistency and of low value, so I don't mind removing it, but I'd like to either understand a good reason for not doing it, or do it for consistency.
libcxx/test/libcxx/utilities/function.objects/refwrap/desugars_to.compile.pass.cpp
Outdated
Show resolved
Hide resolved
…rapper and cv-refs Previously, any cv-ref qualification on an operation would cause __desugars_to to report false, which would lead to unnecessary pessimizations. The same holds for reference_wrapper. In practice, cv-ref qualifications on the operation itself are not relevant to determining whether an operation desugars to something else or not. Fixes llvm#129312
ldionne
force-pushed
the
review/perf-fix-reference_wrapper
branch
from
d2e6185 to
bfd2a23
Compare
ldionne
changed the title
Previously, const and ref qualification on an operation would cause __desugars_to to report false, which would lead to unnecessary pessimizations. The same holds for reference_wrapper.
In practice, const and ref qualifications on the operation itself are not relevant to determining whether an operation desugars to something else or not, so can be ignored.
We are not stripping volatile qualifiers from operations in this patch because we feel that this requires additional discussion.
Fixes #129312