[Mesa-dev] [PATCH 10/23] i965/fs: Remove fs_reg::sechalf.

[Paul Berry]

On 15 January 2014 13:53, Francisco Jerez <currojerez at riseup.net> wrote:

> Paul Berry <stereotype441 at gmail.com> writes:
>
> > On 2 December 2013 11:31, Francisco Jerez <currojerez at riseup.net> wrote:
> >
> >> +/**
> >> + * Get either of the 8-component halves of a 16-component register.
> >> + */
> >> +static inline fs_reg
> >> +half(const fs_reg &reg, unsigned idx)
> >> +{
> >> +   assert(idx == 0 || (reg.file != HW_REG && reg.file != IMM));
> >> +   return byte_offset(reg, 8 * idx * reg.stride * type_sz(reg.type));
> >> +}
> >> +
> >>
> >
> > I'd like to see a comment explaining that it's safe to call this function
> > on a register with 32 bits per component, since brw_reg.h's byte_offset()
> > handles byte offsets greater than 32 by incrementing the register number.
> >
> I've added a comment on the allowed range of fs_reg::subreg_offset here
> [1].  Do you think that's clear enough?
>

The comment you added is good, but it doesn't address my concern.  My
concern is that callers of half() might see the phrase "Get either of the
8-component halves of a 16-component register" and reasonably think this
means "get either of the 8-components halves of a register that stores 16
components in its 256 bits".  Since most 16-wide FS "registers" are
actually register pairs that store 16 components in 512 bits, they might be
misled into thinking that half() doesn't work on these register pairs.
Adding a comment to fs_reg::subreg_offset doesn't help that situation
because it doesn't change the misleading text above the half() function.

How about changing the comment above half() to say this:

/**
 * Get either of the 8-component halves of a 16-component register.
 *
 * Note: this also works if \c reg represents a SIMD16 pair of registers.
 */


not realize that is is safe to call it on a register with 32 bits per
component, because they might erroneously think "oh, this function only
works on 16-component registers; that is, registers that


>
> Thanks.
>
> > Also, for sanity's sake, it would be nice for this function to include
> the
> > assertion:
> >
> >    assert(idx < 2);
> >
> > With those changes, this patch is:
> >
> > Reviewed-by: Paul Berry <stereotype441 at gmail.com>
>
> [1]
> http://cgit.freedesktop.org/~currojerez/mesa/commit/?h=image-load-store&id=14ec9dfd42fec1ee5e5e7f48f98a36fe620cf4e6
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140121/a9e18340/attachment-0001.html>