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

[Kristian Høgsberg]

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.

Kristian

>
> 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.