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

[Iago Toral]

On Thu, 2015-11-19 at 12:08 -0800, Matt Turner wrote:
> 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. :)
> 

Thanks Kristian, I'll use your version and add your Rb and Matt's to it.

Iago