This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] PR libstdc++/91788 improve codegen for std::variant<T...>::index()
On 24/09/19 11:24 +0200, Marc Glisse wrote:
On Tue, 24 Sep 2019, Jonathan Wakely wrote:
On 24/09/19 09:57 +0100, Jonathan Wakely wrote:
On 23/09/19 19:39 +0200, Marc Glisse wrote:
On Mon, 23 Sep 2019, Jonathan Wakely wrote:
If __index_type is a smaller type than size_t, then the result of
size_t(__index_type(-1)) is not equal to size_t(-1), but to an incorrect
value such as size_t(255) or size_t(65535). The old implementation of
variant<T...>::index() uses (size_t(__index_type(_M_index + 1)) - 1)
which is always correct, but generates suboptimal code for many common
cases.
When the __index_type is size_t or valueless variants are not possible
we can just return the value directly.
When the number of alternatives is sufficiently small the result of
converting the _M_index value to the corresponding signed type will be
either non-negative or -1. In those cases converting to the signed type
and then to size_t will either produce the correct positive value or
will sign extend -1 to (size_t)-1 as desired.
For the remaining case we keep the existing arithmetic operations to
ensure the correct result.
PR libstdc++/91788 (partial)
* include/std/variant (variant::index()): Improve codegen for cases
where conversion to size_t already works correctly.
Tested x86_64-linux, committed to trunk.
Thanks.
+ if constexpr (is_same_v<__index_type, size_t>)
+ return this->_M_index;
I don't think this special case is useful, gcc has no trouble
optimizing the other 2 versions to nothing when the types are
the same. Of course it won't hurt either.
My rationale was that it's much cheaper to instantiate is_same_v than
the __never_valueless<T...>() check (and will be even cheaper after
the concepts-cxx2a branch merges, as I plan to make is_same_v use the
__is_same_as built-in to avoid instantiating the std::is_same class
template).
That's probably not a big saving, as the __never_valueless function
template will almost certainly be used by some other member function
anyway.
On the other hand ... a variant with size_t as the index type is
probably vanishingly rare, because it would need tens of thousands of
alternatives.
I thought the code only allowed unsigned char and unsigned short, so
it would require a platform where size_t is the same as one of
those...
For some reason I thought it would use size_t when there are more than
64k alternatives, but we just don't support that. So it's never
size_t.
I'm running the tests for the attached fix.
So doing the (sizeof...(_Types) <= __index_type(-1)/2
case first might make more sense.
Er, from a codegen point of view, I would rather start with the
simplest version (the zero-extension, as in the current code).
--
Marc Glisse
commit 2f997a270eb1360dd3411f4f9a6212fa0dd4e8d6
Author: Jonathan Wakely <jwakely@redhat.com>
Date: Tue Sep 24 11:10:24 2019 +0100
Remove check for impossible condition in std::variant::index()
The __index_type is only ever unsigned char or unsigned short, so not
the same type as size_t.
* include/std/variant (variant::index()): Remove impossible case.
diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant
index c0043243ec2..646ef416272 100644
--- a/libstdc++-v3/include/std/variant
+++ b/libstdc++-v3/include/std/variant
@@ -1520,9 +1520,7 @@ namespace __variant
constexpr size_t index() const noexcept
{
using __index_type = typename _Base::__index_type;
- if constexpr (is_same_v<__index_type, size_t>)
- return this->_M_index;
- else if constexpr (__detail::__variant::__never_valueless<_Types...>())
+ if constexpr (__detail::__variant::__never_valueless<_Types...>())
return this->_M_index;
else if constexpr (sizeof...(_Types) <= __index_type(-1) / 2)
return make_signed_t<__index_type>(this->_M_index);