[Mesa-dev] [PATCH v4 19/44] i965/fs: Use byte_scattered_write on 16-bit store_ssbo
Jason Ekstrand
jason at jlekstrand.net
Fri Dec 1 10:12:58 UTC 2017
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 <jason at jlekstrand.net>
On Wed, Nov 29, 2017 at 6:42 PM, Jose Maria Casanova Crespo <
jmcasanova at igalia.com> wrote:
> From: Alejandro Piñeiro <apinheiro at 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 <jmcasanova at igalia.com>
> Signed-off-by: Alejandro Piñeiro <apinheiro at 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.
> /* 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.
>
> - /* 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?
> + /* 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?
> + /* 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:
assert(num_components * type_size < 16);
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. :)
> + 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171201/f2d28e16/attachment.html>
More information about the mesa-dev
mailing list