Re: [Mesa-dev] [PATCH v3 20/43] i965/fs: Add byte scattered write message and fs support

[Chema Casanova]

On 31/10/17 01:02, Jason Ekstrand wrote:
> On Thu, Oct 12, 2017 at 11:38 AM, Jose Maria Casanova Crespo
> > wrote:
> 
> Signed-off-by: Jose Maria Casanova Crespo  >
> Signed-off-by: Alejandro Piñeiro  >
> ---
>  src/intel/compiler/brw_eu.h                    |  6 ++
>  src/intel/compiler/brw_eu_defines.h            | 17 +
>  src/intel/compiler/brw_eu_emit.c               | 89
> ++
>  src/intel/compiler/brw_fs.cpp                  | 10 +++
>  src/intel/compiler/brw_fs_copy_propagation.cpp |  2 +
>  src/intel/compiler/brw_fs_generator.cpp        |  5 ++
>  src/intel/compiler/brw_fs_surface_builder.cpp  | 17 +
>  src/intel/compiler/brw_fs_surface_builder.h    |  9 +++
>  src/intel/compiler/brw_shader.cpp              |  7 ++
>  9 files changed, 162 insertions(+)
> 
> diff --git a/src/intel/compiler/brw_eu.h b/src/intel/compiler/brw_eu.h
> index 145942a54f..b44ca0f518 100644
> --- a/src/intel/compiler/brw_eu.h
> +++ b/src/intel/compiler/brw_eu.h
> @@ -476,6 +476,12 @@ brw_typed_surface_write(struct brw_codegen *p,
>                          unsigned num_channels);
> 
>  void
> +brw_byte_scattered_write(struct brw_codegen *p,
> +                         struct brw_reg payload,
> +                         struct brw_reg surface,
> +                         unsigned msg_length);
> +
> +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 1751f18293..9aac385ba7 100644
> --- a/src/intel/compiler/brw_eu_defines.h
> +++ b/src/intel/compiler/brw_eu_defines.h
> @@ -390,6 +390,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,
> @@ -1231,4 +1241,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 {
> +   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 8c1e4c5eae..84d85be653 100644
> --- a/src/intel/compiler/brw_eu_emit.c
> +++ b/src/intel/compiler/brw_eu_emit.c
> @@ -2483,6 +2483,49 @@ brw_send_indirect_surface_message(struct
> brw_codegen *p,
>     return insn;
>  }
> 
> +
> +static struct brw_inst *
> +brw_send_indirect_scattered_message(struct brw_codegen *p,
> +                                    unsigned sfid,
> +                                    struct brw_reg dst,
> +                                    struct brw_reg payload,
> +                                    struct brw_reg surface,
> +                                    unsigned message_len,
> +                                    unsigned response_len,
> +                                    bool header_present)
> 
> 
> How is this any different from brw_send_indirect_surface_message?  They
> look identical except for the fact that this one is missing the explicit
> brw_set_default_exec_size I added to the other as part of my subgroup
> series.  If there's no real difference, let's delete this one and just
> use the other.  You can make a pretty good case that the scattered byte
> messages are "surface" messages.

There was no real difference, so I've modified them to use the
brw_send_indirect_surface_message, at the end it just makes sense to
move all scattered_message to the surface_access namespace. Following
the same reasoning.

> +{
> +   const struct gen_device_info *devinfo = p->devinfo;
> +   struct brw_inst *insn;
> +
> +   if (surface.file != BRW_IMMEDIATE_VALUE) {
> +      struct brw_reg addr = retype(brw_address_reg(0),
> BRW_REGISTER_TYPE_UD);
> +
> +      brw_push_insn_state(p);
> +      brw_set_default_access_mode(p, BRW_ALIGN_1);
> +      brw_set_default_mask_control(p, BRW_MASK_DISABLE);
> +      

Re: [Mesa-dev] [PATCH v3 20/43] i965/fs: Add byte scattered write message and fs support

[Jason Ekstrand]

On Thu, Oct 12, 2017 at 11:38 AM, Jose Maria Casanova Crespo <
jmcasan...@igalia.com> wrote:

> Signed-off-by: Jose Maria Casanova Crespo 
> Signed-off-by: Alejandro Piñeiro 
> ---
>  src/intel/compiler/brw_eu.h|  6 ++
>  src/intel/compiler/brw_eu_defines.h| 17 +
>  src/intel/compiler/brw_eu_emit.c   | 89
> ++
>  src/intel/compiler/brw_fs.cpp  | 10 +++
>  src/intel/compiler/brw_fs_copy_propagation.cpp |  2 +
>  src/intel/compiler/brw_fs_generator.cpp|  5 ++
>  src/intel/compiler/brw_fs_surface_builder.cpp  | 17 +
>  src/intel/compiler/brw_fs_surface_builder.h|  9 +++
>  src/intel/compiler/brw_shader.cpp  |  7 ++
>  9 files changed, 162 insertions(+)
>
> diff --git a/src/intel/compiler/brw_eu.h b/src/intel/compiler/brw_eu.h
> index 145942a54f..b44ca0f518 100644
> --- a/src/intel/compiler/brw_eu.h
> +++ b/src/intel/compiler/brw_eu.h
> @@ -476,6 +476,12 @@ brw_typed_surface_write(struct brw_codegen *p,
>  unsigned num_channels);
>
>  void
> +brw_byte_scattered_write(struct brw_codegen *p,
> + struct brw_reg payload,
> + struct brw_reg surface,
> + unsigned msg_length);
> +
> +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 1751f18293..9aac385ba7 100644
> --- a/src/intel/compiler/brw_eu_defines.h
> +++ b/src/intel/compiler/brw_eu_defines.h
> @@ -390,6 +390,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,
> @@ -1231,4 +1241,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 {
> +   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 8c1e4c5eae..84d85be653 100644
> --- a/src/intel/compiler/brw_eu_emit.c
> +++ b/src/intel/compiler/brw_eu_emit.c
> @@ -2483,6 +2483,49 @@ brw_send_indirect_surface_message(struct
> brw_codegen *p,
> return insn;
>  }
>
> +
> +static struct brw_inst *
> +brw_send_indirect_scattered_message(struct brw_codegen *p,
> +unsigned sfid,
> +struct brw_reg dst,
> +struct brw_reg payload,
> +struct brw_reg surface,
> +unsigned message_len,
> +unsigned response_len,
> +bool header_present)
>

How is this any different from brw_send_indirect_surface_message?  They
look identical except for the fact that this one is missing the explicit
brw_set_default_exec_size I added to the other as part of my subgroup
series.  If there's no real difference, let's delete this one and just use
the other.  You can make a pretty good case that the scattered byte
messages are "surface" messages.


> +{
> +   const struct gen_device_info *devinfo = p->devinfo;
> +   struct brw_inst *insn;
> +
> +   if (surface.file != BRW_IMMEDIATE_VALUE) {
> +  struct brw_reg addr = retype(brw_address_reg(0),
> BRW_REGISTER_TYPE_UD);
> +
> +  brw_push_insn_state(p);
> +  brw_set_default_access_mode(p, BRW_ALIGN_1);
> +  brw_set_default_mask_control(p, BRW_MASK_DISABLE);
> +  brw_set_default_predicate_control(p, BRW_PREDICATE_NONE);
> +
> +  /* Mask out invalid bits from the surface index to avoid hangs e.g.
> when
> +   * some surface array is accessed out of bounds.
> +   */
> +  insn = brw_AND(p, addr,
> + suboffset(vec1(retype(surface,
> BRW_REGISTER_TYPE_UD)),
> +   BRW_GET_SWZ(surface.swizzle, 0)),
> + brw_imm_ud(0xff));
> +
> +  brw_pop_insn_state(p);
> +
> +  surface = addr;
> +   }
> +
> +   insn = brw_send_indirect_message(p, sfid, dst, payload, surface);
> +   brw_inst_set_mlen(devinfo, insn, message_len);
> +   brw_inst_set_rlen(devinfo, insn, response_len);
> +   brw_inst_set_header_present(devinfo, insn, header_present);
> +
> +   

[Mesa-dev] [PATCH v3 20/43] i965/fs: Add byte scattered write message and fs support

[Jose Maria Casanova Crespo]

Signed-off-by: Jose Maria Casanova Crespo 
Signed-off-by: Alejandro Piñeiro 
---
 src/intel/compiler/brw_eu.h|  6 ++
 src/intel/compiler/brw_eu_defines.h| 17 +
 src/intel/compiler/brw_eu_emit.c   | 89 ++
 src/intel/compiler/brw_fs.cpp  | 10 +++
 src/intel/compiler/brw_fs_copy_propagation.cpp |  2 +
 src/intel/compiler/brw_fs_generator.cpp|  5 ++
 src/intel/compiler/brw_fs_surface_builder.cpp  | 17 +
 src/intel/compiler/brw_fs_surface_builder.h|  9 +++
 src/intel/compiler/brw_shader.cpp  |  7 ++
 9 files changed, 162 insertions(+)

diff --git a/src/intel/compiler/brw_eu.h b/src/intel/compiler/brw_eu.h
index 145942a54f..b44ca0f518 100644
--- a/src/intel/compiler/brw_eu.h
+++ b/src/intel/compiler/brw_eu.h
@@ -476,6 +476,12 @@ brw_typed_surface_write(struct brw_codegen *p,
 unsigned num_channels);
 
 void
+brw_byte_scattered_write(struct brw_codegen *p,
+ struct brw_reg payload,
+ struct brw_reg surface,
+ unsigned msg_length);
+
+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 1751f18293..9aac385ba7 100644
--- a/src/intel/compiler/brw_eu_defines.h
+++ b/src/intel/compiler/brw_eu_defines.h
@@ -390,6 +390,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,
@@ -1231,4 +1241,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 {
+   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 8c1e4c5eae..84d85be653 100644
--- a/src/intel/compiler/brw_eu_emit.c
+++ b/src/intel/compiler/brw_eu_emit.c
@@ -2483,6 +2483,49 @@ brw_send_indirect_surface_message(struct brw_codegen *p,
return insn;
 }
 
+
+static struct brw_inst *
+brw_send_indirect_scattered_message(struct brw_codegen *p,
+unsigned sfid,
+struct brw_reg dst,
+struct brw_reg payload,
+struct brw_reg surface,
+unsigned message_len,
+unsigned response_len,
+bool header_present)
+{
+   const struct gen_device_info *devinfo = p->devinfo;
+   struct brw_inst *insn;
+
+   if (surface.file != BRW_IMMEDIATE_VALUE) {
+  struct brw_reg addr = retype(brw_address_reg(0), BRW_REGISTER_TYPE_UD);
+
+  brw_push_insn_state(p);
+  brw_set_default_access_mode(p, BRW_ALIGN_1);
+  brw_set_default_mask_control(p, BRW_MASK_DISABLE);
+  brw_set_default_predicate_control(p, BRW_PREDICATE_NONE);
+
+  /* Mask out invalid bits from the surface index to avoid hangs e.g. when
+   * some surface array is accessed out of bounds.
+   */
+  insn = brw_AND(p, addr,
+ suboffset(vec1(retype(surface, BRW_REGISTER_TYPE_UD)),
+   BRW_GET_SWZ(surface.swizzle, 0)),
+ brw_imm_ud(0xff));
+
+  brw_pop_insn_state(p);
+
+  surface = addr;
+   }
+
+   insn = brw_send_indirect_message(p, sfid, dst, payload, surface);
+   brw_inst_set_mlen(devinfo, insn, message_len);
+   brw_inst_set_rlen(devinfo, insn, response_len);
+   brw_inst_set_header_present(devinfo, insn, header_present);
+
+   return insn;
+}
+
 static bool
 while_jumps_before_offset(const struct gen_device_info *devinfo,
   brw_inst *insn, int while_offset, int start_offset)
@@ -2887,6 +2930,52 @@ brw_untyped_surface_write(struct brw_codegen *p,
 }
 
 static void
+brw_set_dp_byte_scattered_write(struct brw_codegen *p,
+struct brw_inst *insn)
+{
+   const struct gen_device_info *devinfo = p->devinfo;
+
+   /* Although we could configure this message to write BYTE, WORD, or DWORD,
+* it was added for the need of writing WORD sizes, so we use directly that
+* size. This could be revisited on the future.
+*/
+   unsigned msg_control = GEN7_BYTE_SCATTERED_DATA_SIZE_WORD