<div dir="ltr"><div><div><div>Assuming the changes you describe below, this patch would get a </div>Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> </div>I think that's enough for you to land the UBO/SSBO portion of 16bit storage. Feel free to send v2 versions of any patches you want me to look at again before you push but I think it's in pretty good shape. </div>--Jason <div><div><div><div><div class="gmail_extra"> <div class="gmail_quote">On Fri, Dec 1, 2017 at 1:21 PM, Chema Casanova <<a href="mailto:jmcasanova@igalia.com" target="_blank">jmcasanova@igalia.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 30/11/17 21:42, Jason Ekstrand wrote: > On Wed, Nov 29, 2017 at 6:08 PM, Jose Maria Casanova Crespo > <<a href="mailto:jmcasanova@igalia.com">jmcasanova@igalia.com</a> <mailto:<a href="mailto:jmcasanova@igalia.com">jmcasanova@igalia.com</a>>> wrote: > > v2: (Jason Ekstrand) > - Enable bit_size parameter to scattered messages to enable > different > bitsizes byte/word/dword. > - Remove use of brw_send_indirect_scattered_message in favor of > brw_send_indirect_surface_message. > - Move scattered messages to surface messages namespace. > - Assert align1 for scattered messages and assume Gen8+. > - Inline brw_set_dp_byte_scattered_write. > > Signed-off-by: Jose Maria Casanova Crespo <<a href="mailto:jmcasanova@igalia.com">jmcasanova@igalia.com</a> > <mailto:<a href="mailto:jmcasanova@igalia.com">jmcasanova@igalia.com</a>>> > Signed-off-by: Alejandro Piñeiro <<a href="mailto:apinheiro@igalia.com">apinheiro@igalia.com</a> > <mailto:<a href="mailto:apinheiro@igalia.com">apinheiro@igalia.com</a>>> <div><div class="h5">> --- > src/intel/compiler/brw_eu.h | 7 +++++ > src/intel/compiler/brw_eu_defines.h | 17 +++++++++++ > src/intel/compiler/brw_eu_emit.c | 42 > ++++++++++++++++++++++++++ > src/intel/compiler/brw_fs.cpp | 14 +++++++++ > src/intel/compiler/brw_fs_copy_propagation.cpp | 2 ++ > src/intel/compiler/brw_fs_generator.cpp | 6 ++++ > src/intel/compiler/brw_fs_surface_builder.cpp | 11 +++++++ > src/intel/compiler/brw_fs_surface_builder.h | 7 +++++ > src/intel/compiler/brw_shader.cpp | 7 +++++ > 9 files changed, 113 insertions(+) > > diff --git a/src/intel/compiler/brw_eu.h b/src/intel/compiler/brw_eu.h > index 343dcd867d..3ac3b4342a 100644 > --- a/src/intel/compiler/brw_eu.h > +++ b/src/intel/compiler/brw_eu.h > @@ -485,6 +485,13 @@ brw_typed_surface_write(struct brw_codegen *p, > unsigned msg_length, > unsigned num_channels); > > +void > +brw_byte_scattered_write(struct brw_codegen *p, > + struct brw_reg payload, > + struct brw_reg surface, > + unsigned msg_length, > + unsigned bit_size); > + > void > brw_memory_fence(struct brw_codegen *p, > struct brw_reg dst); > diff --git a/src/intel/compiler/brw_eu_defines.h > b/src/intel/compiler/brw_eu_defines.h > index 9d5cf05c86..de6330ee54 100644 > --- a/src/intel/compiler/brw_eu_defines.h > +++ b/src/intel/compiler/brw_eu_defines.h > @@ -402,6 +402,16 @@ enum opcode { > > SHADER_OPCODE_RND_MODE, > > + /** > + * Byte scattered write/read opcodes. > + * > + * LOGICAL opcodes are eventually translated to the matching > non-LOGICAL > + * opcode, but instead of taking a single payload blog they > expect their > + * arguments separately as individual sources, like untyped > write/read. > + */ > + SHADER_OPCODE_BYTE_SCATTERED_WRITE, > + SHADER_OPCODE_BYTE_SCATTERED_WRITE_LOGICAL, > + > SHADER_OPCODE_MEMORY_FENCE, > > SHADER_OPCODE_GEN4_SCRATCH_READ, > @@ -1255,4 +1265,11 @@ enum PACKED brw_rnd_mode { > BRW_RND_MODE_UNSPECIFIED, /* Unspecified rounding mode */ > }; > > +/* MDC_DS - Data Size Message Descriptor Control Field */ > +enum PACKED brw_data_size { > > > I'm not sure how I feel about this being an enum with such a generic name. </div></div>Right, PRM use a more exactly "Data Elements" but this field only used byte_scattered/scaled writes/reads. As I will follow your next suggestion of using #define, I'm chaging the name to: #define GEN7_BYTE_SCATTERED_DATA_ELEMENT_BYTE 0 #define GEN7_BYTE_SCATTERED_DATA_ELEMENT_WORD 1 #define GEN7_BYTE_SCATTERED_DATA_ELEMENT_DWORD 2 I'll include in the comment about MSC_DS "Specifies the number of Bytes to be read or written per Dword used at byte_scattered read/write and byte_scaled read/write messages." > > > + GEN7_BYTE_SCATTERED_DATA_SIZE_BYTE = 0, > + GEN7_BYTE_SCATTERED_DATA_SIZE_WORD = 1, > + GEN7_BYTE_SCATTERED_DATA_SIZE_DWORD = 2 > +}; > + > #endif /* BRW_EU_DEFINES_H */ > diff --git a/src/intel/compiler/brw_eu_emit.c > b/src/intel/compiler/brw_eu_emit.c > index ca97ff7325..ded7e228cf 100644 > --- a/src/intel/compiler/brw_eu_emit.c > +++ b/src/intel/compiler/brw_eu_emit.c > @@ -2580,6 +2580,7 @@ brw_send_indirect_surface_message(struct > brw_codegen *p, > return insn; > } > > + > static bool > while_jumps_before_offset(const struct gen_device_info *devinfo, > brw_inst *insn, int while_offset, int > start_offset) > @@ -2983,6 +2984,47 @@ brw_untyped_surface_write(struct brw_codegen *p, > p, insn, num_channels); > } > > +static enum brw_data_size brw_data_size_from_bit_size(unsigned > bit_size) > > > Please put the return type on it's own line. OK. It will be unsigned. and I'm changing the name of the function according to brw_scattered_data_element_from_bit_size to maintain coherence with "enum" values. <div><div class="h5"> > +{ > + switch (bit_size) { > + case 8: > + return GEN7_BYTE_SCATTERED_DATA_SIZE_BYTE; > + case 16: > + return GEN7_BYTE_SCATTERED_DATA_SIZE_WORD; > + case 32: > + return GEN7_BYTE_SCATTERED_DATA_SIZE_DWORD; > + default: > + unreachable("Unsupported bit_size for byte scattered messages"); > + } > +} > + > +void > +brw_byte_scattered_write(struct brw_codegen *p, > + struct brw_reg payload, > + struct brw_reg surface, > + unsigned msg_length, > + unsigned bit_size) > +{ > + assert(brw_inst_access_mode(p->devinfo, p->current) == BRW_ALIGN_1); > + const struct gen_device_info *devinfo = p->devinfo; > + const unsigned sfid = GEN7_SFID_DATAPORT_DATA_CACHE; > + > + struct brw_inst *insn = brw_send_indirect_surface_message( > + p, sfid, brw_writemask(brw_null_reg(), WRITEMASK_XYZW), > + payload, surface, msg_length, 0, true); > + > + unsigned msg_control = brw_data_size_from_bit_size(bit_size) << 2; > > > I have no idea what C is going to do with types when you have a packed > enum and you then provide a shift. I think I would be more comfortable > if you used #defines and had brw_data_size_from_bit_size return an unsigned. </div></div>If packed enum uses a char as type, it this emum has only 0,1,2 values the shiff would fit in the char, and then it would be converted to uint32. But I can not confirm what each different compiler would do. So let's go with the simple choice and use #defines, what about changing the name of the function brw_byte_scattered_data_element_from_bit_size. > + > + if (brw_inst_exec_size(devinfo, p->current) == BRW_EXECUTE_16) > + msg_control |= 1; > + else > + msg_control |= 0; > + > + brw_inst_set_dp_msg_type(devinfo, insn, > + > HSW_DATAPORT_DC_PORT0_BYTE_SCATTERED_WRITE); > > > Should we assert at the top that devinfo->gen > 7 || devinfo->is_haswell? Yes, I thought that when asserting for ALIGN1 we were already limitting that really. But yes, I'll include it. <div><div class="h5"> > + brw_inst_set_dp_msg_control(devinfo, insn, msg_control); > +} > + > static void > brw_set_dp_typed_atomic_message(struct brw_codegen *p, > struct brw_inst *insn, > diff --git a/src/intel/compiler/brw_fs.cpp > b/src/intel/compiler/brw_fs.cpp > index 36fb337c62..32f1d757f0 100644 > --- a/src/intel/compiler/brw_fs.cpp > +++ b/src/intel/compiler/brw_fs.cpp > @@ -250,6 +250,7 @@ fs_inst::is_send_from_grf() const > case SHADER_OPCODE_UNTYPED_ATOMIC: > case SHADER_OPCODE_UNTYPED_SURFACE_READ: > case SHADER_OPCODE_UNTYPED_SURFACE_WRITE: > + case SHADER_OPCODE_BYTE_SCATTERED_WRITE: > case SHADER_OPCODE_TYPED_ATOMIC: > case SHADER_OPCODE_TYPED_SURFACE_READ: > case SHADER_OPCODE_TYPED_SURFACE_WRITE: > @@ -749,6 +750,11 @@ fs_inst::components_read(unsigned i) const > else > return 1; > > + case SHADER_OPCODE_BYTE_SCATTERED_WRITE_LOGICAL: > + assert(src[3].file == IMM && > + src[4].file == IMM); > + return 1; > + > case SHADER_OPCODE_UNTYPED_ATOMIC_LOGICAL: > case SHADER_OPCODE_TYPED_ATOMIC_LOGICAL: { > assert(src[3].file == IMM && > @@ -791,6 +797,7 @@ fs_inst::size_read(int arg) const > case SHADER_OPCODE_TYPED_SURFACE_READ: > case SHADER_OPCODE_TYPED_SURFACE_WRITE: > case FS_OPCODE_INTERPOLATE_AT_PER_SLOT_OFFSET: > + case SHADER_OPCODE_BYTE_SCATTERED_WRITE: > if (arg == 0) > return mlen * REG_SIZE; > break; > @@ -4538,6 +4545,12 @@ fs_visitor::lower_logical_sends() > ibld.sample_mask_reg()); > break; > > + case SHADER_OPCODE_BYTE_SCATTERED_WRITE_LOGICAL: > + lower_surface_logical_send(ibld, inst, > + SHADER_OPCODE_BYTE_SCATTERED_WRITE, > + ibld.sample_mask_reg()); > + break; > + > case SHADER_OPCODE_UNTYPED_ATOMIC_LOGICAL: > lower_surface_logical_send(ibld, inst, > SHADER_OPCODE_UNTYPED_ATOMIC, > @@ -5022,6 +5035,7 @@ get_lowered_simd_width(const struct > gen_device_info *devinfo, > case SHADER_OPCODE_UNTYPED_ATOMIC_LOGICAL: > case SHADER_OPCODE_UNTYPED_SURFACE_READ_LOGICAL: > case SHADER_OPCODE_UNTYPED_SURFACE_WRITE_LOGICAL: > + case SHADER_OPCODE_BYTE_SCATTERED_WRITE_LOGICAL: > return MIN2(16, inst->exec_size); > > case SHADER_OPCODE_URB_READ_SIMD8: > diff --git a/src/intel/compiler/brw_fs_copy_propagation.cpp > b/src/intel/compiler/brw_fs_copy_propagation.cpp > index cb11739608..fcf4706b7a 100644 > --- a/src/intel/compiler/brw_fs_copy_propagation.cpp > +++ b/src/intel/compiler/brw_fs_copy_propagation.cpp > @@ -655,6 +655,7 @@ fs_visitor::try_constant_propagate(fs_inst > *inst, acp_entry *entry) > case SHADER_OPCODE_TYPED_ATOMIC: > case SHADER_OPCODE_TYPED_SURFACE_READ: > case SHADER_OPCODE_TYPED_SURFACE_WRITE: > + case SHADER_OPCODE_BYTE_SCATTERED_WRITE: > /* We only propagate into the surface argument of the > * instruction. Everything else goes through LOAD_PAYLOAD. > */ > @@ -694,6 +695,7 @@ fs_visitor::try_constant_propagate(fs_inst > *inst, acp_entry *entry) > case SHADER_OPCODE_TYPED_ATOMIC_LOGICAL: > case SHADER_OPCODE_TYPED_SURFACE_READ_LOGICAL: > case SHADER_OPCODE_TYPED_SURFACE_WRITE_LOGICAL: > + case SHADER_OPCODE_BYTE_SCATTERED_WRITE_LOGICAL: > inst->src[i] = val; > progress = true; > break; > diff --git a/src/intel/compiler/brw_fs_generator.cpp > b/src/intel/compiler/brw_fs_generator.cpp > index 1835c4bf72..fedc9acf97 100644 > --- a/src/intel/compiler/brw_fs_generator.cpp > +++ b/src/intel/compiler/brw_fs_generator.cpp > @@ -2073,6 +2073,12 @@ fs_generator::generate_code(const cfg_t *cfg, > int dispatch_width) > inst->mlen, src[2].ud); > break; > > + case SHADER_OPCODE_BYTE_SCATTERED_WRITE: > + assert(src[2].file == BRW_IMMEDIATE_VALUE); > + brw_byte_scattered_write(p, src[0], src[1], > + inst->mlen, src[2].ud); > + break; > + > case SHADER_OPCODE_TYPED_ATOMIC: > assert(src[2].file == BRW_IMMEDIATE_VALUE); > brw_typed_atomic(p, dst, src[0], src[1], > diff --git a/src/intel/compiler/brw_fs_surface_builder.cpp > b/src/intel/compiler/brw_fs_surface_builder.cpp > index d00d8920b2..37cc29e361 100644 > --- a/src/intel/compiler/brw_fs_surface_builder.cpp > +++ b/src/intel/compiler/brw_fs_surface_builder.cpp > @@ -160,6 +160,16 @@ namespace brw { > return emit_send(bld, SHADER_OPCODE_TYPED_ATOMIC_LOGICAL, > addr, tmp, surface, dims, op, rsize); > } > + > + void > + emit_byte_scattered_write(const fs_builder &bld, const fs_reg > &surface, > + const fs_reg &addr, const fs_reg &src, > + unsigned dims, unsigned size, > + unsigned bit_size, brw_predicate pred) > + { > + emit_send(bld, SHADER_OPCODE_BYTE_SCATTERED_WRITE_LOGICAL, > + addr, src, surface, dims, bit_size, 0, pred); > + } > } > } > > @@ -1192,3 +1202,4 @@ namespace brw { > } > } > } > + > diff --git a/src/intel/compiler/brw_fs_surface_builder.h > b/src/intel/compiler/brw_fs_surface_builder.h > index 32b56d387f..bf9a8c68c8 100644 > --- a/src/intel/compiler/brw_fs_surface_builder.h > +++ b/src/intel/compiler/brw_fs_surface_builder.h > @@ -63,6 +63,13 @@ namespace brw { > const fs_reg &src0, const fs_reg &src1, > unsigned dims, unsigned rsize, unsigned op, > brw_predicate pred = BRW_PREDICATE_NONE); > + > + void > + emit_byte_scattered_write(const fs_builder &bld, const fs_reg > &surface, > + const fs_reg &addr, const fs_reg &src, > + unsigned dims, unsigned size, > + unsigned bit_size, > + brw_predicate pred = > BRW_PREDICATE_NONE); > } > > namespace image_access { > diff --git a/src/intel/compiler/brw_shader.cpp > b/src/intel/compiler/brw_shader.cpp > index d7d7616cf4..209552e1b2 100644 > --- a/src/intel/compiler/brw_shader.cpp > +++ b/src/intel/compiler/brw_shader.cpp > @@ -293,6 +293,11 @@ brw_instruction_name(const struct > gen_device_info *devinfo, enum opcode op) > case SHADER_OPCODE_MEMORY_FENCE: > return "memory_fence"; > > + case SHADER_OPCODE_BYTE_SCATTERED_WRITE: > + return "byte_scattered_write"; > + case SHADER_OPCODE_BYTE_SCATTERED_WRITE_LOGICAL: > + return "byte_scattered_write_logical"; > + > case SHADER_OPCODE_LOAD_PAYLOAD: > return "load_payload"; > case FS_OPCODE_PACK: > @@ -963,6 +968,8 @@ backend_instruction::has_side_effects() const > case SHADER_OPCODE_GEN4_SCRATCH_WRITE: > case SHADER_OPCODE_UNTYPED_SURFACE_WRITE: > case SHADER_OPCODE_UNTYPED_SURFACE_WRITE_LOGICAL: > + case SHADER_OPCODE_BYTE_SCATTERED_WRITE: > + case SHADER_OPCODE_BYTE_SCATTERED_WRITE_LOGICAL: > case SHADER_OPCODE_TYPED_ATOMIC: > case SHADER_OPCODE_TYPED_ATOMIC_LOGICAL: > case SHADER_OPCODE_TYPED_SURFACE_WRITE: > -- > 2.14.3 > > _______________________________________________ > mesa-dev mailing list </div></div>> <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <mailto:<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a>> > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > <<a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a>> > > </blockquote></div> </div></div></div></div></div></div>