[PATCH] PR libstdc++/91788 improve codegen for std::variant<T...>::index()
Marc Glisse
marc.glisse@inria.fr
Tue Sep 24 09:25:00 GMT 2019
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...
> 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
More information about the Gcc-patches
mailing list