[Mesa-dev] [PATCH (gles3) 20/20] i965/fs/gen7: Emit code for GLSL 3.00 pack/unpack operations (v2)

[Eric Anholt]

Chad Versace <chad.versace at linux.intel.com> writes:

> On 01/21/2013 01:18 PM, Eric Anholt wrote:
>> Chad Versace <chad.versace at linux.intel.com> writes:
>>>     ir->remove();
>>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_emit.cpp b/src/mesa/drivers/dri/i965/brw_fs_emit.cpp
>>> index 324e665..0ff296c 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_fs_emit.cpp
>>> +++ b/src/mesa/drivers/dri/i965/brw_fs_emit.cpp
>>> @@ -923,6 +923,34 @@ fs_generator::generate_set_global_offset(fs_inst *inst,
>>>  }
>>>  
>>>  void
>>> +fs_generator::generate_unpack_half_2x16_split_y(fs_inst *inst,
>>> +                                                struct brw_reg dst,
>>> +                                                struct brw_reg src)
>>> +{
>>> +   assert(intel->gen >= 7);
>>> +
>>> +   /* src has the form of unpackHalf2x16's input:
>>> +    *
>>> +    *         w     z     y          x
>>> +    *   |undef|undef|undef|0xhhhhllll|
>> 
>> I don't understand what "w z y x" mean here.  I thought src was just a
>> scalar uint (one uint per pixel).
>
> You're right. I had vec4 on the mind when I wrote the comment. How about
> changing it to this?
>
>    /* Each channel of src has the form of unpackHalf2x16's input: 0xhhhhllll.
>     * We wish to access only the "hhhh" bits of each channel, and hence
>     * must access the register with a 16 bit subregister offset.  To do so, we
>     * must halve the size of the source data type from UD to UW and compensate
>     * by doubling the stride.
>     */

Looks good!

>>> +   assert(src.type == BRW_REGISTER_TYPE_UD);
>>> +   src.type = BRW_REGISTER_TYPE_UW;
>>> +   if (src.vstride > 0)
>>> +      ++src.vstride;
>>> +   if (src.hstride > 0)
>>> +      ++src.hstride;
>> 
>> This ++hstride/++vstride looks totally wrong to me.  We use a vstride of
>> 8 in 16-wide for temporaries -- doesn't this totally trash that?
>> 
>> I think what you want here is an unconditional *= 2 for both values.
>
> This indeed does *= 2, from the hardware's perspective. My mistake for
> not providing an explanation.
>
> src.hstride, before the adjustment, is one of the BRE_HORIZONTAL_STRIDE_*
> values, which are defined as
>
> #define BRW_HORIZONTAL_STRIDE_0   0
> #define BRW_HORIZONTAL_STRIDE_1   1
> #define BRW_HORIZONTAL_STRIDE_2   2
> #define BRW_HORIZONTAL_STRIDE_4   3

Oh, right.  I'd love to switch which type of value brw_reg uses some day
-- the enums are irritating to work with, and I've made this particular
mistake more than once.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130121/f7705271/attachment.pgp>