Assuming the changes you describe below, this patch would get a Reviewed-by: Jason Ekstrand <[email protected]>
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. --Jason On Fri, Dec 1, 2017 at 1:21 PM, Chema Casanova <[email protected]> wrote: > On 30/11/17 21:42, Jason Ekstrand wrote: > > On Wed, Nov 29, 2017 at 6:08 PM, Jose Maria Casanova Crespo > > <[email protected] <mailto:[email protected]>> 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 <[email protected] > > <mailto:[email protected]>> > > Signed-off-by: Alejandro Piñeiro <[email protected] > > <mailto:[email protected]>> > > --- > > 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. > > 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. > > > > +{ > > + 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. > > 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. > > > + 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 > > [email protected] <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 [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
