<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Dec 4, 2017 at 4:50 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:45, Jason Ekstrand wrote: > On Wed, Nov 29, 2017 at 6:50 PM, Jose Maria Casanova Crespo <div><div class="h5">> <<a href="mailto:jmcasanova@igalia.com">jmcasanova@igalia.com</a> <mailto:<a href="mailto:jmcasanova@igalia.com">jmcasanova@igalia.com</a>>> wrote: > > v2: Fix alignment style (Topi Pohjolainen) > (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_read. > --- > src/intel/compiler/brw_eu.h | 8 +++++++ > src/intel/compiler/brw_eu_defines.h | 2 ++ > src/intel/compiler/brw_eu_emit.c | 30 > ++++++++++++++++++++++++++ > src/intel/compiler/brw_fs.cpp | 19 ++++++++++++++++ > 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 | 6 ++++++ > 9 files changed, 90 insertions(+), 1 deletion(-) > > diff --git a/src/intel/compiler/brw_eu.h b/src/intel/compiler/brw_eu.h > index 3ac3b4342a..2d0f56f793 100644 > --- a/src/intel/compiler/brw_eu.h > +++ b/src/intel/compiler/brw_eu.h > @@ -485,6 +485,14 @@ brw_typed_surface_write(struct brw_codegen *p, > unsigned msg_length, > unsigned num_channels); > > +void > +brw_byte_scattered_read(struct brw_codegen *p, > + struct brw_reg dst, > + struct brw_reg payload, > + struct brw_reg surface, > + unsigned msg_length, > + unsigned bit_size); > + > void > brw_byte_scattered_write(struct brw_codegen *p, > struct brw_reg payload, > diff --git a/src/intel/compiler/brw_eu_defines.h > b/src/intel/compiler/brw_eu_defines.h > index de6330ee54..aa510ebfa4 100644 > --- a/src/intel/compiler/brw_eu_defines.h > +++ b/src/intel/compiler/brw_eu_defines.h > @@ -409,6 +409,8 @@ enum opcode { > * 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_READ, > + SHADER_OPCODE_BYTE_SCATTERED_READ_LOGICAL, > SHADER_OPCODE_BYTE_SCATTERED_WRITE, > SHADER_OPCODE_BYTE_SCATTERED_WRITE_LOGICAL, > > diff --git a/src/intel/compiler/brw_eu_emit.c > b/src/intel/compiler/brw_eu_emit.c > index ded7e228cf..bdc516848a 100644 > --- a/src/intel/compiler/brw_eu_emit.c > +++ b/src/intel/compiler/brw_eu_emit.c > @@ -2998,6 +2998,36 @@ static enum brw_data_size > brw_data_size_from_bit_size(unsigned bit_size) > } > } > > + > +void > +brw_byte_scattered_read(struct brw_codegen *p, > + struct brw_reg dst, > + 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, dst, payload, surface, msg_length, > + brw_surface_payload_size(p, 1, true, true), > + false); > + > + unsigned msg_control = brw_data_size_from_bit_size(bit_size) << 2; > + > + if (brw_inst_exec_size(devinfo, p->current) == BRW_EXECUTE_16) > + msg_control |= 1; /* SIMD16 mode */ > + else > + msg_control |= 0; /* SIMD8 mode */ > + > + brw_inst_set_dp_msg_type(devinfo, insn, > + HSW_DATAPORT_DC_PORT0_BYTE_SCATTERED_READ); > + brw_inst_set_dp_msg_control(devinfo, insn, msg_control); > +} > + > void > brw_byte_scattered_write(struct brw_codegen *p, > struct brw_reg payload, > diff --git a/src/intel/compiler/brw_fs.cpp > b/src/intel/compiler/brw_fs.cpp > index 32f1d757f0..1ca4d416b2 100644 > --- a/src/intel/compiler/brw_fs.cpp > +++ b/src/intel/compiler/brw_fs.cpp > @@ -251,6 +251,7 @@ fs_inst::is_send_from_grf() const > case SHADER_OPCODE_UNTYPED_SURFACE_READ: > case SHADER_OPCODE_UNTYPED_SURFACE_WRITE: > case SHADER_OPCODE_BYTE_SCATTERED_WRITE: > + case SHADER_OPCODE_BYTE_SCATTERED_READ: > case SHADER_OPCODE_TYPED_ATOMIC: > case SHADER_OPCODE_TYPED_SURFACE_READ: > case SHADER_OPCODE_TYPED_SURFACE_WRITE: > @@ -750,6 +751,16 @@ fs_inst::components_read(unsigned i) const > else > return 1; > > + case SHADER_OPCODE_BYTE_SCATTERED_READ_LOGICAL: > + assert(src[3].file == IMM && > + src[4].file == IMM); > + if (i == 0) > + return 1; > + else if (i == 1) > + return 0; > > > I suppose src[1] is for the data to write which is unused for this > opcode? If so, I'd rather we just do > > /* src[1] is normally for data to write and is unused */ > return i == 1 ? 0 : 1; </div></div>Yes, that is the reason, in this case as it is a read operation, it isn't using the source at the payload, the only special change with ' other surfaces ops is that we are using src[4] as IMM passing the bitsize for the scattered read, and dimension should always be 1, so the source would be always 1 component for write operations. Would it make sense to include a comment with the explanation of the params to explain why we use src[4]? Something like: /* Scattered logical opcodes use the following params: * src[0] Surface coordinates * src[1] Surface operation source (ignored for reads) * src[2] Surface * src[3] IMM with always 1 dimension. * src[4] IMM with arg bitsize for scattered read/write 8, 16, 32 */ </blockquote><div> </div><div>That would be fine with me.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> With your change, Can I consider this patch as R-b? <div><div class="h5"></div></div></blockquote><div> </div><div>Sure. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5"> > + else > + return 1; > + > case SHADER_OPCODE_BYTE_SCATTERED_WRITE_LOGICAL: > assert(src[3].file == IMM && > src[4].file == IMM); > @@ -798,6 +809,7 @@ fs_inst::size_read(int arg) const > case SHADER_OPCODE_TYPED_SURFACE_WRITE: > case FS_OPCODE_INTERPOLATE_AT_PER_SLOT_OFFSET: > case SHADER_OPCODE_BYTE_SCATTERED_WRITE: > + case SHADER_OPCODE_BYTE_SCATTERED_READ: > if (arg == 0) > return mlen * REG_SIZE; > break; > @@ -4545,6 +4557,12 @@ fs_visitor::lower_logical_sends() > ibld.sample_mask_reg()); > break; > > + case SHADER_OPCODE_BYTE_SCATTERED_READ_LOGICAL: > + lower_surface_logical_send(ibld, inst, > + SHADER_OPCODE_BYTE_SCATTERED_READ, > + fs_reg()); > + break; > + > case SHADER_OPCODE_BYTE_SCATTERED_WRITE_LOGICAL: > lower_surface_logical_send(ibld, inst, > SHADER_OPCODE_BYTE_SCATTERED_WRITE, > @@ -5036,6 +5054,7 @@ get_lowered_simd_width(const struct > gen_device_info *devinfo, > case SHADER_OPCODE_UNTYPED_SURFACE_READ_LOGICAL: > case SHADER_OPCODE_UNTYPED_SURFACE_WRITE_LOGICAL: > case SHADER_OPCODE_BYTE_SCATTERED_WRITE_LOGICAL: > + case SHADER_OPCODE_BYTE_SCATTERED_READ_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 fcf4706b7a..d4d01d783c 100644 > --- a/src/intel/compiler/brw_fs_copy_propagation.cpp > +++ b/src/intel/compiler/brw_fs_copy_propagation.cpp > @@ -656,6 +656,7 @@ fs_visitor::try_constant_propagate(fs_inst > *inst, acp_entry *entry) > case SHADER_OPCODE_TYPED_SURFACE_READ: > case SHADER_OPCODE_TYPED_SURFACE_WRITE: > case SHADER_OPCODE_BYTE_SCATTERED_WRITE: > + case SHADER_OPCODE_BYTE_SCATTERED_READ: > /* We only propagate into the surface argument of the > * instruction. Everything else goes through LOAD_PAYLOAD. > */ > @@ -696,6 +697,7 @@ fs_visitor::try_constant_propagate(fs_inst > *inst, acp_entry *entry) > case SHADER_OPCODE_TYPED_SURFACE_READ_LOGICAL: > case SHADER_OPCODE_TYPED_SURFACE_WRITE_LOGICAL: > case SHADER_OPCODE_BYTE_SCATTERED_WRITE_LOGICAL: > + case SHADER_OPCODE_BYTE_SCATTERED_READ_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 fedc9acf97..a3861cd68e 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_READ: > + assert(src[2].file == BRW_IMMEDIATE_VALUE); > + brw_byte_scattered_read(p, dst, src[0], src[1], > + 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], > diff --git a/src/intel/compiler/brw_fs_surface_builder.cpp > b/src/intel/compiler/brw_fs_surface_builder.cpp > index 37cc29e361..c346ef9e70 100644 > --- a/src/intel/compiler/brw_fs_surface_builder.cpp > +++ b/src/intel/compiler/brw_fs_surface_builder.cpp > @@ -161,6 +161,16 @@ namespace brw { > addr, tmp, surface, dims, op, rsize); > } > > + fs_reg > + emit_byte_scattered_read(const fs_builder &bld, > + const fs_reg &surface, const fs_reg > &addr, > + unsigned dims, unsigned size, > + unsigned bit_size, brw_predicate pred) > + { > + return emit_send(bld, > SHADER_OPCODE_BYTE_SCATTERED_READ_LOGICAL, > + addr, fs_reg(), surface, dims, bit_size, > size, pred); > + } > + > void > emit_byte_scattered_write(const fs_builder &bld, const fs_reg > &surface, > const fs_reg &addr, const fs_reg &src, > @@ -1202,4 +1212,3 @@ namespace brw { > } > } > } > - > diff --git a/src/intel/compiler/brw_fs_surface_builder.h > b/src/intel/compiler/brw_fs_surface_builder.h > index bf9a8c68c8..194d61d489 100644 > --- a/src/intel/compiler/brw_fs_surface_builder.h > +++ b/src/intel/compiler/brw_fs_surface_builder.h > @@ -64,6 +64,13 @@ namespace brw { > unsigned dims, unsigned rsize, unsigned op, > brw_predicate pred = BRW_PREDICATE_NONE); > > + fs_reg > + emit_byte_scattered_read(const fs_builder &bld, > + const fs_reg &surface, const fs_reg > &addr, > + unsigned dims, unsigned size, > + unsigned bit_size, > + 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, > diff --git a/src/intel/compiler/brw_shader.cpp > b/src/intel/compiler/brw_shader.cpp > index 209552e1b2..74b52976d7 100644 > --- a/src/intel/compiler/brw_shader.cpp > +++ b/src/intel/compiler/brw_shader.cpp > @@ -293,6 +293,10 @@ 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_READ: > + return "byte_scattered_read"; > + case SHADER_OPCODE_BYTE_SCATTERED_READ_LOGICAL: > + return "byte_scattered_read_logical"; > case SHADER_OPCODE_BYTE_SCATTERED_WRITE: > return "byte_scattered_write"; > case SHADER_OPCODE_BYTE_SCATTERED_WRITE_LOGICAL: > @@ -999,6 +1003,8 @@ backend_instruction::is_volatile() const > case SHADER_OPCODE_UNTYPED_SURFACE_READ_LOGICAL: > case SHADER_OPCODE_TYPED_SURFACE_READ: > case SHADER_OPCODE_TYPED_SURFACE_READ_LOGICAL: > + case SHADER_OPCODE_BYTE_SCATTERED_READ: > + case SHADER_OPCODE_BYTE_SCATTERED_READ_LOGICAL: > case SHADER_OPCODE_URB_READ_SIMD8: > case SHADER_OPCODE_URB_READ_SIMD8_PER_SLOT: > case VEC4_OPCODE_URB_READ: > -- > 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>> <div class="HOEnZb"><div class="h5">> > > > > _______________________________________________ > mesa-dev mailing list > <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> > </div></div></blockquote></div> </div></div>