On Fri, Dec 1, 2017 at 2:46 AM, Chema Casanova <jmcasan...@igalia.com> wrote:
> On 01/12/17 11:12, Jason Ekstrand wrote: > > I've left some comments below that I think clean things up and make this > > better, but I believe it is correct as-is. > > > > Reviewed-by: Jason Ekstrand <ja...@jlekstrand.net > > <mailto:ja...@jlekstrand.net>> > > > > On Wed, Nov 29, 2017 at 6:42 PM, Jose Maria Casanova Crespo > > <jmcasan...@igalia.com <mailto:jmcasan...@igalia.com>> wrote: > > > > From: Alejandro Piñeiro <apinhe...@igalia.com > > <mailto:apinhe...@igalia.com>> > > > > We need to rely on byte scattered writes as untyped writes are 32-bit > > size. We could try to keep using 32-bit messages when we have two or > > four 16-bit elements, but for simplicity sake, we use the same > message > > for any component number. We revisit this aproach in the follwing > > patches. > > > > v2: Removed use of stride = 2 on 16-bit sources (Jason Ekstrand) > > > > v3: (Jason Ekstrand) > > - Include bit_size to scattered write message and remove > namespace > > - specific for scattered messages. > > - Move comment to proper place. > > - Squashed with i965/fs: Adjust type_size/type_slots on > store_ssbo. > > (Jose Maria Casanova) > > - Take into account that get_nir_src returns now WORD types for > > 16-bit sources instead of DWORD. > > > > Signed-off-by: Jose Maria Casanova Crespo <jmcasan...@igalia.com > > <mailto:jmcasan...@igalia.com>> > > Signed-off-by: Alejandro Piñeiro <apinhe...@igalia.com > > <mailto:apinhe...@igalia.com>> > > --- > > src/intel/compiler/brw_fs_nir.cpp | 51 > > ++++++++++++++++++++++++++++----------- > > 1 file changed, 37 insertions(+), 14 deletions(-) > > > > diff --git a/src/intel/compiler/brw_fs_nir.cpp > > b/src/intel/compiler/brw_fs_nir.cpp > > index d6ab286147..ff04e2468b 100644 > > --- a/src/intel/compiler/brw_fs_nir.cpp > > +++ b/src/intel/compiler/brw_fs_nir.cpp > > @@ -4075,14 +4075,15 @@ fs_visitor::nir_emit_intrinsic(const > > fs_builder &bld, nir_intrinsic_instr *instr > > * Also, we have to suffle 64-bit data to be in the > > appropriate layout > > * expected by our 32-bit write messages. > > */ > > - unsigned type_size = 4; > > - if (nir_src_bit_size(instr->src[0]) == 64) { > > - type_size = 8; > > + unsigned bit_size = nir_src_bit_size(instr->src[0]); > > + unsigned type_size = bit_size / 8; > > + if (bit_size == 64) { > > val_reg = shuffle_64bit_data_for_32bit_write(bld, > > val_reg, instr->num_components); > > } > > > > - unsigned type_slots = type_size / 4; > > + /* 16-bit types would use a minimum of 1 slot */ > > + unsigned type_slots = MAX2(type_size / 4, 1); > > > > > > Given that this is only used for emit_typed_write, maybe we should just > > move it next to the emit_typed_write call and just get rid of the > > MAX2(). More on that later. > > It makes sanes, i follow partially this approach at "[PATCH v4 26/44] > i965/fs: Optimize 16-bit SSBO stores by packing two into a 32-bit reg" > using an slots_per_component that is just 2 for 64-bits and 1 for the > other bitsizes. But i like your approach. > > > /* Combine groups of consecutive enabled channels in one write > > * message. We use ffs to find the first enabled channel and > > then ffs on > > @@ -4093,12 +4094,19 @@ fs_visitor::nir_emit_intrinsic(const > > fs_builder &bld, nir_intrinsic_instr *instr > > unsigned first_component = ffs(writemask) - 1; > > unsigned length = ffs(~(writemask >> first_component)) - 1; > > > > > > If the one above is first_component, num_components would be a better > > name for this one. It's very confusing go have something generically > > named "length" in a piece of code with so many different possible units. > > It was also confussing to me. What about a rename to > num_consecutive_components as that what is really calculating? so we > don't confuse it with the num_components of instr. > Hrm... That would work I suppose. Not a huge deal in any case. > > - /* We can't write more than 2 64-bit components at once. > > Limit the > > - * length of the write to what we can do and let the next > > iteration > > - * handle the rest > > - */ > > - if (type_size > 4) > > + if (type_size > 4) { > > + /* We can't write more than 2 64-bit components at > > once. Limit > > + * the length of the write to what we can do and let > > the next > > + * iteration handle the rest. > > + */ > > length = MIN2(2, length); > > + } else if (type_size == 2) { > > > > > > Maybe type_size < 4? > > I should have advanced this change to this patch, you commented it > already for current [PATCH v4 26/44] > > > > + /* For 16-bit types we are using byte scattered writes, > > that can > > + * only write one component per call. So we limit the > > length, and > > + * let the write happening in several iterations. > > + */ > > + length = 1; > > + } > > > > fs_reg offset_reg; > > nir_const_value *const_offset = > > nir_src_as_const_value(instr->src[2]); > > @@ -4112,11 +4120,26 @@ fs_visitor::nir_emit_intrinsic(const > > fs_builder &bld, nir_intrinsic_instr *instr > > brw_imm_ud(type_size * first_component)); > > } > > > > - > > - emit_untyped_write(bld, surf_index, offset_reg, > > - offset(val_reg, bld, first_component * > > type_slots), > > - 1 /* dims */, length * type_slots, > > - BRW_PREDICATE_NONE); > > + if (type_size == 2) { > > > > > > maybe type_size < 4? > > Agree > > > + /* Untyped Surface messages have a fixed 32-bit size, > > so we need > > + * to rely on byte scattered in order to write 16-bit > > elements. > > + * The byte_scattered_write message needs that every > > written 16-bit > > + * type to be aligned 32-bits (stride=2). > > + */ > > + fs_reg tmp = bld.vgrf(BRW_REGISTER_TYPE_D); > > + bld.MOV(subscript(tmp, BRW_REGISTER_TYPE_W, 0), > > + offset(val_reg, bld, first_component)); > > + emit_byte_scattered_write(bld, surf_index, offset_reg, > > + tmp, > > + 1 /* dims */, 1, > > + bit_size, > > + BRW_PREDICATE_NONE); > > + } else { > > > > > > If we moved type_slots here, I think we could very nicely future-proof > > things as follows: > > I think that we can not move it here because it is really used when > calculating the offset for the components of 64-bits, > > > assert(num_components * type_size < 16); > > Shouldn't it be <= > > > assert((num_components * type_size) % 4 == 0); > > assert((first_component * type_size) % 4 == 0); > > unsigned first_slot = (first_component * type_size) / 4; > > unsigned num_slots = (num_components * type_size) / 4; > > emit_untyped_write(bld, surf_index, reg_offset, > > offset(val_reg, bld, first_slot), > > 1 /* dims */, num_slots, > > BRW_PREDICATE_NONE); > > > > That said, let's not get ahead of ourselves. This can all be done as a > > later clean-up on top of the optimization patch if that's easier. :) > > We were thinking this with the following optimization in the past. But > at that moment we thought it was easier to understand doing it in two > steps. I'll wait for you to review [PATCH v4 26/44] and try to advance > as much as I can to this first one to avoid re-writing lines. That's fine. After reading your later optimization patch, it seems like it lands in a reasonable state. Forget this comment if you'd like. > > > + emit_untyped_write(bld, surf_index, offset_reg, > > + offset(val_reg, bld, first_component > > * type_slots), > > + 1 /* dims */, length * type_slots, > > + BRW_PREDICATE_NONE); > > + } > > > > /* Clear the bits in the writemask that we just wrote, > > then try > > * again to see if more channels are left. > > -- > > 2.14.3 > > > > _______________________________________________ > > mesa-dev mailing list > > mesa-dev@lists.freedesktop.org <mailto:mesa-dev@lists. > freedesktop.org> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > > <https://lists.freedesktop.org/mailman/listinfo/mesa-dev> > > > > >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
