<div dir="ltr">On 15 January 2014 13:53, Francisco Jerez <span dir="ltr"><<a href="mailto:currojerez@riseup.net" target="_blank">currojerez@riseup.net</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="im">Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> writes:<br>

<br>
> On 2 December 2013 11:31, Francisco Jerez <<a href="mailto:currojerez@riseup.net">currojerez@riseup.net</a>> wrote:<br>
><br>
>> +/**<br>
>> + * Get either of the 8-component halves of a 16-component register.<br>
>> + */<br>
>> +static inline fs_reg<br>
>> +half(const fs_reg &reg, unsigned idx)<br>
>> +{<br>
>> +   assert(idx == 0 || (reg.file != HW_REG && reg.file != IMM));<br>
>> +   return byte_offset(reg, 8 * idx * reg.stride * type_sz(reg.type));<br>
>> +}<br>
>> +<br>
>><br>
><br>
> I'd like to see a comment explaining that it's safe to call this function<br>
> on a register with 32 bits per component, since brw_reg.h's byte_offset()<br>
> handles byte offsets greater than 32 by incrementing the register number.<br>
><br>
</div>I've added a comment on the allowed range of fs_reg::subreg_offset here<br>
[1].  Do you think that's clear enough?<br></blockquote><div><br></div><div>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.<br>
<br></div><div>How about changing the comment above half() to say this:<br><br>/**<br> * Get either of the 8-component halves of a 16-component register.<br> *<br> * Note: this also works if \c reg represents a SIMD16 pair of registers.<br>
 */<br><br></div><div><br>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 <br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Thanks.<br>
<div class="im"><br>
> Also, for sanity's sake, it would be nice for this function to include the<br>
> assertion:<br>
><br>
>    assert(idx < 2);<br>
><br>
> With those changes, this patch is:<br>
><br>
> Reviewed-by: Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>><br>
<br>
</div>[1] <a href="http://cgit.freedesktop.org/~currojerez/mesa/commit/?h=image-load-store&id=14ec9dfd42fec1ee5e5e7f48f98a36fe620cf4e6" target="_blank">http://cgit.freedesktop.org/~currojerez/mesa/commit/?h=image-load-store&id=14ec9dfd42fec1ee5e5e7f48f98a36fe620cf4e6</a><br>

</blockquote></div><br></div></div>