[PATCH] PR libstdc++/91788 improve codegen for std::variant<T...>::index()

[Jonathan Wakely]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch.txt
Type: text/x-patch
Size: 1106 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/libstdc++/attachments/20190924/c414480d/attachment.bin>