[Mesa-dev] [PATCH v4 18/44] i965/fs: Add byte scattered write message and fs support

Jason Ekstrand jason at jlekstrand.net
Fri Dec 1 23:40:21 UTC 2017


Assuming the changes you describe below, this patch would get a

Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>

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 <jmcasanova at igalia.com>
wrote:

> On 30/11/17 21:42, Jason Ekstrand wrote:
> > On Wed, Nov 29, 2017 at 6:08 PM, Jose Maria Casanova Crespo
> > <jmcasanova at igalia.com <mailto:jmcasanova at igalia.com>> 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 <jmcasanova at igalia.com
> >     <mailto:jmcasanova at igalia.com>>
> >     Signed-off-by: Alejandro PiƱeiro <apinheiro at igalia.com
> >     <mailto:apinheiro at igalia.com>>
> >     ---
> >      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
> >     mesa-dev at lists.freedesktop.org <mailto:mesa-dev at lists.
> freedesktop.org>
> >     https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >     <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/f916a296/attachment-0001.html>


More information about the mesa-dev mailing list