On 15 January 2014 13:53, Francisco Jerez <curroje...@riseup.net> wrote:
> Paul Berry <stereotype...@gmail.com> writes:
>
> > On 2 December 2013 11:31, Francisco Jerez <curroje...@riseup.net> wrote:
> >
> >> +/**
> >> + * Get either of the 8-component halves of a 16-component register.
> >> + */
> >> +static inline fs_reg
> >> +half(const fs_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 <stereotype...@gmail.com>
>
> [1]
> http://cgit.freedesktop.org/~currojerez/mesa/commit/?h=image-load-store&id=14ec9dfd42fec1ee5e5e7f48f98a36fe620cf4e6
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
