[Mesa-dev] [PATCH 20/20] nir: Narrow unnecessary 64-bit operations to 32-bits

[Connor Abbott]

On Wed, Jul 19, 2017 at 1:37 PM, Matt Turner <mattst88 at gmail.com> wrote:
> On Mon, Jul 17, 2017 at 8:14 PM, Connor Abbott <cwabbott0 at gmail.com> wrote:
>>
>> This function seems to be doing two subtly different things:
>>
>> 1. If the instruction defining the source is nir_op_pack_64_2x32_split
>> or the source type is nir_type_uint, then it checks if the high 32
>> bits are 0.
>> 2. Otherwise, it checks that the value can be replaced by a 32-bit
>> value that's sign extended to 64 bits, that is, whether the high 32
>> bits are the same as bit 31.
>>
>> The problem is that if the source type is nir_type_int, then this
>> function checks two different things depending on whether the source
>> is a constant or the result of nir_op_pack_64_2x32_split. Based on how
>> you're using it, it seems like you really want to check whether the
>> high bits are the same as bit 31 in this case, so need to bail out of
>> the first 'if' if the source type isn't nir_type_uint.
>
> You are right. I gave this a try, and it breaks the optimization,
> because the cases I'm looking to optimize are (ine, 0,
> pack_64_2x32(ballot(...), 0)). Since we use "ine", so the type comes
> back as nir_type_int. In that case I have
>
>     case nir_type_int:
>        if (!hi_bits || !lo_bits ||
>            hi_bits->i32[i] != (lo_bits->i32[i] >> 31))
>           return false;
>        break;
>
> but lo_bits is not const, so it fails.
>
> Any suggestions?
>
> I don't think I really had any reason for the sign-extending case, FWIW.

Hmm. Maybe we should split this into two functions, say,
is_zero_extended_32_bits and is_sign_extended_32_bits? nir_type_int
vs. nir_type_uint is really about whether it makes sense to add a
negation modifier to the instruction's source, which is rarely
something we care about in generic optimizations since modifiers don't
exist then and the driver might opt not to use them at all. Usually,
the only input type we care about in optimizations is nir_type_bool,
since it actually tells you something about the input (that it's
either 0 or ~0). If you don't care about the sign-extending case, then
you can just rename the function and drop the switching on
input_types.