On Sat, Jun 14, 2014 at 12:54 PM, Kenneth Graunke <kenn...@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 <kenn...@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. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
