[Mesa-dev] [PATCH 3/9] i965: fix 64-bit immediates in brw_inst(_set)_bits

[Matt Turner]

On Thu, Nov 19, 2015 at 9:30 AM, Kristian Høgsberg <krh at bitplanet.net> wrote:
> On Thu, Nov 19, 2015 at 2:05 AM, Iago Toral Quiroga <itoral at igalia.com> wrote:
>> From: Connor Abbott <connor.w.abbott at intel.com>
>>
>> If we tried to get/set something that was exactly 64 bits, we would
>> try to do (1 << 64) - 1 to calculate the mask which doesn't give us all
>> 1's like we want.
>>
>> v2 (Iago)
>>  - Replace ~0 by ~0ull
>>  - Removed unnecessary parenthesis
>>
>> Reviewed-by: Iago Toral Quiroga <itoral at igalia.com>
>> ---
>>  src/mesa/drivers/dri/i965/brw_inst.h | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_inst.h b/src/mesa/drivers/dri/i965/brw_inst.h
>> index 4ed95c4..ec08194 100644
>> --- a/src/mesa/drivers/dri/i965/brw_inst.h
>> +++ b/src/mesa/drivers/dri/i965/brw_inst.h
>> @@ -694,7 +694,8 @@ brw_inst_bits(const brw_inst *inst, unsigned high, unsigned low)
>>     high %= 64;
>>     low %= 64;
>>
>> -   const uint64_t mask = (1ull << (high - low + 1)) - 1;
>> +   const uint64_t mask = (high - low == 63) ? ~0ull :
>> +      (1ull << (high - low + 1)) - 1;
>
> Can we do
>
> const uint64_t mask = (~0ul >> (64 - (high - low + 1)));
>
> instead?

I don't think so, because ~0ul is of type unsigned, so right shifting
it shifts in zeros. I was going to make a similar comment on the
original patch -- "-1" is preferable over ~0u with an increasingly
long sequence of l's because it's signed, so it's sign extended to
fill whatever you assign it to. In your code though, since it's an
operand we'd need -1ll, I think...

So with s/~0ul/-1ll/, I think that'll work? It's all evaluated at
compile-time in any case, so clarity is the only metric. I don't have
a preference.