[Mesa-dev] [PATCH 11/19] i965: Convert brw_eu_compact.c to the new brw_inst API.

[Matt Turner]

On Sat, Jun 14, 2014 at 12:54 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
>> -   uint16_t compacted, uncompacted = 0;
>> -
>> -   uncompacted |= (src->bits2.ud >> 13) & 0xfff;
>> +   uint16_t compacted;
>> +   uint16_t uncompacted =                           /* 12b */
>> +      (brw_inst_src0_vstride(brw, src)      << 8) | /* 4b */
>> +      (brw_inst_src0_width(brw, src)        << 5) | /* 3b */
>> +      (brw_inst_src0_hstride(brw, src)      << 3) | /* 2b */
>
> One thing that's a little funny here...we pull out hstride/width/vstride,
> which makes sense for align1 mode...but presumably this function is also used
> for align16 mode, where we instead have src0_da16_swiz_x etc.
>
> But, it's the same bits, so this ought to work.  I'm not objecting, it's
> just...a little funny at first glance.
>
> I don't think it's worth adding conditionals, but would it be worth adding a
> comment saying basically /* this also works for align16 mode because they
> share the same bits */?
>
> Whatever you decide is fine.  This patch is:
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

I'm glad you mentioned this. I think I want to modify these to use
brw_inst_bits/brw_inst_set_bits instead. I know they're different on
Broadwell, so we'll have slightly more C, but I think the benefits
outweight that.

The SandyBridge docs give only the bitfields, while the IVB/HSW docs
give the bitfields and a useless list of out of order field names
(that regularly alias each other). So you have to rely on the bitfield
definitions anyway to make sense of it.

Also, the fields corresponding to the indicies are typically
contiguous. For instance, 16 of the 17 bits of SNB's control index
table are consecutive, so using brw_inst_bits lets us grab those
directly, without confusing the compiler into generating garbage by
extracting them separately, shifting, and oring them back together.

   text   data    bss    dec    hex filename
 4118      0     32   4150   1036 brw_eu_compact.o    before conversion
 6774      0     32   6806   1a96 brw_eu_compact.o    after conversion
(this patch)
 4662      0     32   4694   1256 brw_eu_compact.o    after bitfield patch

There's evidently some overhead in the new approach. I've got an idea
how to fix it that I'm not sure is worth implementing, but I think
using the bit getter and setter is definitely the right way to go
here.