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

[Matt Turner]

On Thu, Nov 19, 2015 at 11:35 AM, Kristian Høgsberg <krh at bitplanet.net> wrote:
> On Thu, Nov 19, 2015 at 11:24 AM, Matt Turner <mattst88 at gmail.com> wrote:
>> 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...
>
> No, shifting in zeros is the whole point. We start out with 64 1 bits,
> then shift it down enough that we end up with (high - low + 1) 1 bits
> at the bottom, which is what we're trying to compute.

Doh. Of course. :)