[03/23] intel/eu: Use brw_set_desc() along with a helper to set common descriptor controls.

Submitted by Francisco Jerez on June 12, 2018, 2:25 a.m.

Details

Message ID 20180612022615.3653-4-currojerez@riseup.net
State New
Headers show
Series "intel/eu: Define SET_BITS helper more easily reusable than SET_FIELD." ( rev: 6 5 4 3 2 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Francisco Jerez June 12, 2018, 2:25 a.m.
This replaces brw_set_message_descriptor() with the composition of
brw_set_desc() and a new inline helper function that packs the common
message descriptor controls into an integer.  The goal is to represent
all message descriptors as a 32-bit integer which is written at once
into the instruction, which is more flexible (SENDS anyone?), robust
(see d2eecf0b0b24d203d0f171807681dffd830d54de fixing an issue
ultimately caused by some bits of the extended message descriptor
being left undefined) and future-proof than the current approach of
specifying the individual descriptor fields directly into the
instruction.

This approach also seems more self-documenting, since it will allow
removing calls to functions with way too many arguments like
brw_set_*_message() and brw_send_indirect_message(), and instead
provide a single descriptor argument constructed from an appropriate
combination of brw_*_desc() helpers.

Note that because brw_set_message_descriptor() was (conditionally?)
overriding fields of the instruction which strictly speaking weren't
part of the message descriptor, this involves calling
brw_inst_set_sfid() and brw_inst_set_eot() in some cases in addition
to brw_set_desc().
---
 src/intel/compiler/brw_eu.h               |  29 +++++---
 src/intel/compiler/brw_eu_emit.c          | 108 +++++++++++-------------------
 src/intel/compiler/brw_vec4_generator.cpp |  17 +++--
 3 files changed, 68 insertions(+), 86 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/intel/compiler/brw_eu.h b/src/intel/compiler/brw_eu.h
index 5a396339fde..b2b20713e45 100644
--- a/src/intel/compiler/brw_eu.h
+++ b/src/intel/compiler/brw_eu.h
@@ -256,14 +256,6 @@  void brw_set_sampler_message(struct brw_codegen *p,
                              unsigned simd_mode,
                              unsigned return_format);
 
-void brw_set_message_descriptor(struct brw_codegen *p,
-                                brw_inst *inst,
-                                enum brw_message_target sfid,
-                                unsigned msg_length,
-                                unsigned response_length,
-                                bool header_present,
-                                bool end_of_thread);
-
 void brw_set_dp_read_message(struct brw_codegen *p,
 			     brw_inst *insn,
 			     unsigned binding_table_index,
@@ -287,6 +279,27 @@  void brw_set_dp_write_message(struct brw_codegen *p,
 			      unsigned end_of_thread,
 			      unsigned send_commit_msg);
 
+/**
+ * Construct a message descriptor immediate with the specified common
+ * descriptor controls.
+ */
+static inline uint32_t
+brw_message_desc(const struct gen_device_info *devinfo,
+                 unsigned msg_length,
+                 unsigned response_length,
+                 bool header_present)
+{
+   if (devinfo->gen >= 5) {
+      return (msg_length << 25 |
+              response_length << 20 |
+              header_present << 19);
+   } else {
+      return (msg_length << 20 |
+              response_length << 16);
+   }
+}
+
+
 void brw_urb_WRITE(struct brw_codegen *p,
 		   struct brw_reg dest,
 		   unsigned msg_reg_nr,
diff --git a/src/intel/compiler/brw_eu_emit.c b/src/intel/compiler/brw_eu_emit.c
index ab9af718152..7389e8e3bde 100644
--- a/src/intel/compiler/brw_eu_emit.c
+++ b/src/intel/compiler/brw_eu_emit.c
@@ -359,48 +359,6 @@  brw_set_src1(struct brw_codegen *p, brw_inst *inst, struct brw_reg reg)
    }
 }
 
-/**
- * Set the Message Descriptor and Extended Message Descriptor fields
- * for SEND messages.
- *
- * \note This zeroes out the Function Control bits, so it must be called
- *       \b before filling out any message-specific data.  Callers can
- *       choose not to fill in irrelevant bits; they will be zero.
- */
-void
-brw_set_message_descriptor(struct brw_codegen *p,
-			   brw_inst *inst,
-			   enum brw_message_target sfid,
-			   unsigned msg_length,
-			   unsigned response_length,
-			   bool header_present,
-			   bool end_of_thread)
-{
-   const struct gen_device_info *devinfo = p->devinfo;
-
-   brw_set_src1(p, inst, brw_imm_d(0));
-
-   /* For indirect sends, `inst` will not be the SEND/SENDC instruction
-    * itself; instead, it will be a MOV/OR into the address register.
-    *
-    * In this case, we avoid setting the extended message descriptor bits,
-    * since they go on the later SEND/SENDC instead and if set here would
-    * instead clobber the conditionalmod bits.
-    */
-   unsigned opcode = brw_inst_opcode(devinfo, inst);
-   if (opcode == BRW_OPCODE_SEND || opcode == BRW_OPCODE_SENDC) {
-      brw_inst_set_sfid(devinfo, inst, sfid);
-   }
-
-   brw_inst_set_mlen(devinfo, inst, msg_length);
-   brw_inst_set_rlen(devinfo, inst, response_length);
-   brw_inst_set_eot(devinfo, inst, end_of_thread);
-
-   if (devinfo->gen >= 5) {
-      brw_inst_set_header_present(devinfo, inst, header_present);
-   }
-}
-
 /**
  * Specify the descriptor and extended descriptor immediate for a SEND(C)
  * message instruction.
@@ -453,9 +411,10 @@  static void brw_set_math_message( struct brw_codegen *p,
       break;
    }
 
+   brw_set_desc(p, inst, brw_message_desc(
+                   devinfo, msg_length, response_length, false));
 
-   brw_set_message_descriptor(p, inst, BRW_SFID_MATH,
-			      msg_length, response_length, false, false);
+   brw_inst_set_sfid(devinfo, inst, BRW_SFID_MATH);
    brw_inst_set_math_msg_function(devinfo, inst, function);
    brw_inst_set_math_msg_signed_int(devinfo, inst, integer_type);
    brw_inst_set_math_msg_precision(devinfo, inst, low_precision);
@@ -473,8 +432,11 @@  static void brw_set_ff_sync_message(struct brw_codegen *p,
 {
    const struct gen_device_info *devinfo = p->devinfo;
 
-   brw_set_message_descriptor(p, insn, BRW_SFID_URB,
-			      1, response_length, true, end_of_thread);
+   brw_set_desc(p, insn, brw_message_desc(
+                   devinfo, 1, response_length, true));
+
+   brw_inst_set_sfid(devinfo, insn, BRW_SFID_URB);
+   brw_inst_set_eot(devinfo, insn, end_of_thread);
    brw_inst_set_urb_opcode(devinfo, insn, 1); /* FF_SYNC */
    brw_inst_set_urb_allocate(devinfo, insn, allocate);
    /* The following fields are not used by FF_SYNC: */
@@ -498,9 +460,11 @@  static void brw_set_urb_message( struct brw_codegen *p,
    assert(devinfo->gen < 7 || !(flags & BRW_URB_WRITE_ALLOCATE));
    assert(devinfo->gen >= 7 || !(flags & BRW_URB_WRITE_PER_SLOT_OFFSET));
 
-   brw_set_message_descriptor(p, insn, BRW_SFID_URB,
-			      msg_length, response_length, true,
-                              flags & BRW_URB_WRITE_EOT);
+   brw_set_desc(p, insn, brw_message_desc(
+                   devinfo, msg_length, response_length, true));
+
+   brw_inst_set_sfid(devinfo, insn, BRW_SFID_URB);
+   brw_inst_set_eot(devinfo, insn, !!(flags & BRW_URB_WRITE_EOT));
 
    if (flags & BRW_URB_WRITE_OWORD) {
       assert(msg_length == 2); /* header + one OWORD of data */
@@ -543,9 +507,11 @@  brw_set_dp_write_message(struct brw_codegen *p,
    const unsigned sfid = (devinfo->gen >= 6 ? target_cache :
                           BRW_SFID_DATAPORT_WRITE);
 
-   brw_set_message_descriptor(p, insn, sfid, msg_length, response_length,
-			      header_present, end_of_thread);
+   brw_set_desc(p, insn, brw_message_desc(
+                   devinfo, msg_length, response_length, header_present));
 
+   brw_inst_set_sfid(devinfo, insn, sfid);
+   brw_inst_set_eot(devinfo, insn, !!end_of_thread);
    brw_inst_set_binding_table_index(devinfo, insn, binding_table_index);
    brw_inst_set_dp_write_msg_type(devinfo, insn, msg_type);
    brw_inst_set_dp_write_msg_control(devinfo, insn, msg_control);
@@ -573,9 +539,12 @@  brw_set_dp_read_message(struct brw_codegen *p,
    const unsigned sfid = (devinfo->gen >= 6 ? target_cache :
                           BRW_SFID_DATAPORT_READ);
 
-   brw_set_message_descriptor(p, insn, sfid, msg_length, response_length,
-			      header_present, false);
+   brw_set_desc(p, insn, brw_message_desc(
+                   devinfo, msg_length, response_length, header_present));
 
+   const unsigned opcode = brw_inst_opcode(devinfo, insn);
+   if (opcode == BRW_OPCODE_SEND || opcode == BRW_OPCODE_SENDC)
+      brw_inst_set_sfid(devinfo, insn, sfid);
    brw_inst_set_binding_table_index(devinfo, insn, binding_table_index);
    brw_inst_set_dp_read_msg_type(devinfo, insn, msg_type);
    brw_inst_set_dp_read_msg_control(devinfo, insn, msg_control);
@@ -597,9 +566,12 @@  brw_set_sampler_message(struct brw_codegen *p,
 {
    const struct gen_device_info *devinfo = p->devinfo;
 
-   brw_set_message_descriptor(p, inst, BRW_SFID_SAMPLER, msg_length,
-			      response_length, header_present, false);
+   brw_set_desc(p, inst, brw_message_desc(
+                   devinfo, msg_length, response_length, header_present));
 
+   const unsigned opcode = brw_inst_opcode(devinfo, inst);
+   if (opcode == BRW_OPCODE_SEND || opcode == BRW_OPCODE_SENDC)
+      brw_inst_set_sfid(devinfo, inst, BRW_SFID_SAMPLER);
    brw_inst_set_binding_table_index(devinfo, inst, binding_table_index);
    brw_inst_set_sampler(devinfo, inst, sampler);
    brw_inst_set_sampler_msg_type(devinfo, inst, msg_type);
@@ -628,8 +600,10 @@  gen7_set_dp_scratch_message(struct brw_codegen *p,
    const unsigned block_size = (devinfo->gen >= 8 ? _mesa_logbase2(num_regs) :
                                 num_regs - 1);
 
-   brw_set_message_descriptor(p, inst, GEN7_SFID_DATAPORT_DATA_CACHE,
-                              mlen, rlen, header_present, false);
+   brw_set_desc(p, inst, brw_message_desc(
+                   devinfo, mlen, rlen, header_present));
+
+   brw_inst_set_sfid(devinfo, inst, GEN7_SFID_DATAPORT_DATA_CACHE);
    brw_inst_set_dp_category(devinfo, inst, 1); /* Scratch Block Read/Write msgs */
    brw_inst_set_scratch_read_write(devinfo, inst, write);
    brw_inst_set_scratch_type(devinfo, inst, dword);
@@ -3307,11 +3281,10 @@  brw_set_memory_fence_message(struct brw_codegen *p,
 {
    const struct gen_device_info *devinfo = p->devinfo;
 
-   brw_set_message_descriptor(p, insn, sfid,
-                              1 /* message length */,
-                              (commit_enable ? 1 : 0) /* response length */,
-                              true /* header present */,
-                              false);
+   brw_set_desc(p, insn, brw_message_desc(
+                   devinfo, 1, (commit_enable ? 1 : 0), true));
+
+   brw_inst_set_sfid(devinfo, insn, sfid);
 
    switch (sfid) {
    case GEN6_SFID_DATAPORT_RENDER_CACHE:
@@ -3679,7 +3652,8 @@  void brw_shader_time_add(struct brw_codegen *p,
    brw_set_src0(p, send, brw_vec1_reg(payload.file,
                                       payload.nr, 0));
    brw_set_src1(p, send, brw_imm_ud(0));
-   brw_set_message_descriptor(p, send, sfid, 2, 0, false, false);
+   brw_set_desc(p, send, brw_message_desc(p->devinfo, 2, 0, false));
+   brw_inst_set_sfid(p->devinfo, send, sfid);
    brw_inst_set_binding_table_index(p->devinfo, send, surf_index);
    brw_set_dp_untyped_atomic_message(p, send, BRW_AOP_ADD, false);
 
@@ -3704,13 +3678,9 @@  brw_barrier(struct brw_codegen *p, struct brw_reg src)
    brw_set_dest(p, inst, retype(brw_null_reg(), BRW_REGISTER_TYPE_UW));
    brw_set_src0(p, inst, src);
    brw_set_src1(p, inst, brw_null_reg());
+   brw_set_desc(p, inst, brw_message_desc(devinfo, 1, 0, false));
 
-   brw_set_message_descriptor(p, inst, BRW_SFID_MESSAGE_GATEWAY,
-                              1 /* msg_length */,
-                              0 /* response_length */,
-                              false /* header_present */,
-                              false /* end_of_thread */);
-
+   brw_inst_set_sfid(devinfo, inst, BRW_SFID_MESSAGE_GATEWAY);
    brw_inst_set_gateway_notify(devinfo, inst, 1);
    brw_inst_set_gateway_subfuncid(devinfo, inst,
                                   BRW_MESSAGE_GATEWAY_SFID_BARRIER_MSG);
diff --git a/src/intel/compiler/brw_vec4_generator.cpp b/src/intel/compiler/brw_vec4_generator.cpp
index 7519ccc9df3..386b071cfda 100644
--- a/src/intel/compiler/brw_vec4_generator.cpp
+++ b/src/intel/compiler/brw_vec4_generator.cpp
@@ -777,10 +777,9 @@  generate_tcs_urb_write(struct brw_codegen *p,
    brw_inst *send = brw_next_insn(p, BRW_OPCODE_SEND);
    brw_set_dest(p, send, brw_null_reg());
    brw_set_src0(p, send, urb_header);
+   brw_set_desc(p, send, brw_message_desc(devinfo, inst->mlen, 0, true));
 
-   brw_set_message_descriptor(p, send, BRW_SFID_URB,
-                              inst->mlen /* mlen */, 0 /* rlen */,
-                              true /* header */, false /* eot */);
+   brw_inst_set_sfid(devinfo, send, BRW_SFID_URB);
    brw_inst_set_urb_opcode(devinfo, send, BRW_URB_OPCODE_WRITE_OWORD);
    brw_inst_set_urb_global_offset(devinfo, send, inst->offset);
    if (inst->urb_write_flags & BRW_URB_WRITE_EOT) {
@@ -953,9 +952,9 @@  generate_vec4_urb_read(struct brw_codegen *p,
    brw_set_dest(p, send, dst);
    brw_set_src0(p, send, header);
 
-   brw_set_message_descriptor(p, send, BRW_SFID_URB,
-                              1 /* mlen */, 1 /* rlen */,
-                              true /* header */, false /* eot */);
+   brw_set_desc(p, send, brw_message_desc(devinfo, 1, 1, true));
+
+   brw_inst_set_sfid(devinfo, send, BRW_SFID_URB);
    brw_inst_set_urb_opcode(devinfo, send, BRW_URB_OPCODE_READ_OWORD);
    brw_inst_set_urb_swizzle_control(devinfo, send, BRW_URB_SWIZZLE_INTERLEAVE);
    brw_inst_set_urb_per_slot_offset(devinfo, send, 1);
@@ -989,9 +988,9 @@  generate_tcs_release_input(struct brw_codegen *p,
    brw_inst *send = brw_next_insn(p, BRW_OPCODE_SEND);
    brw_set_dest(p, send, brw_null_reg());
    brw_set_src0(p, send, header);
-   brw_set_message_descriptor(p, send, BRW_SFID_URB,
-                              1 /* mlen */, 0 /* rlen */,
-                              true /* header */, false /* eot */);
+   brw_set_desc(p, send, brw_message_desc(devinfo, 1, 0, true));
+
+   brw_inst_set_sfid(devinfo, send, BRW_SFID_URB);
    brw_inst_set_urb_opcode(devinfo, send, BRW_URB_OPCODE_READ_OWORD);
    brw_inst_set_urb_complete(devinfo, send, 1);
    brw_inst_set_urb_swizzle_control(devinfo, send, is_unpaired.ud ?

Comments

On Monday, June 11, 2018 7:25:55 PM PDT Francisco Jerez wrote:
> This replaces brw_set_message_descriptor() with the composition of
> brw_set_desc() and a new inline helper function that packs the common
> message descriptor controls into an integer.  The goal is to represent
> all message descriptors as a 32-bit integer which is written at once
> into the instruction, which is more flexible (SENDS anyone?), robust
> (see d2eecf0b0b24d203d0f171807681dffd830d54de fixing an issue
> ultimately caused by some bits of the extended message descriptor
> being left undefined) and future-proof than the current approach of
> specifying the individual descriptor fields directly into the
> instruction.
> 
> This approach also seems more self-documenting, since it will allow
> removing calls to functions with way too many arguments like
> brw_set_*_message() and brw_send_indirect_message(), and instead
> provide a single descriptor argument constructed from an appropriate
> combination of brw_*_desc() helpers.
> 
> Note that because brw_set_message_descriptor() was (conditionally?)
> overriding fields of the instruction which strictly speaking weren't
> part of the message descriptor, this involves calling
> brw_inst_set_sfid() and brw_inst_set_eot() in some cases in addition
> to brw_set_desc().
> ---
>  src/intel/compiler/brw_eu.h               |  29 +++++---
>  src/intel/compiler/brw_eu_emit.c          | 108 +++++++++++-------------------
>  src/intel/compiler/brw_vec4_generator.cpp |  17 +++--
>  3 files changed, 68 insertions(+), 86 deletions(-)
> 
> diff --git a/src/intel/compiler/brw_eu.h b/src/intel/compiler/brw_eu.h
> index 5a396339fde..b2b20713e45 100644
> --- a/src/intel/compiler/brw_eu.h
> +++ b/src/intel/compiler/brw_eu.h
> @@ -256,14 +256,6 @@ void brw_set_sampler_message(struct brw_codegen *p,
>                               unsigned simd_mode,
>                               unsigned return_format);
>  
> -void brw_set_message_descriptor(struct brw_codegen *p,
> -                                brw_inst *inst,
> -                                enum brw_message_target sfid,
> -                                unsigned msg_length,
> -                                unsigned response_length,
> -                                bool header_present,
> -                                bool end_of_thread);
> -
>  void brw_set_dp_read_message(struct brw_codegen *p,
>  			     brw_inst *insn,
>  			     unsigned binding_table_index,
> @@ -287,6 +279,27 @@ void brw_set_dp_write_message(struct brw_codegen *p,
>  			      unsigned end_of_thread,
>  			      unsigned send_commit_msg);
>  
> +/**
> + * Construct a message descriptor immediate with the specified common
> + * descriptor controls.
> + */
> +static inline uint32_t
> +brw_message_desc(const struct gen_device_info *devinfo,
> +                 unsigned msg_length,
> +                 unsigned response_length,
> +                 bool header_present)
> +{

Perhaps it would be good to add

      assert(msg_length >= 1 && msg_length <= 15);

> +   if (devinfo->gen >= 5) {

      assert(response_length <= 16);


> +      return (msg_length << 25 |
> +              response_length << 20 |
> +              header_present << 19);
> +   } else {

      assert(response_length <= 8);

I'm not so concerned with validating the values here, just thinking it
might make sense to verify that mlen fits in a U4, for example, so we
don't accidentally bleed over into other fields when encoding it.

I suppose the validator already catches this, though...

> +      return (msg_length << 20 |
> +              response_length << 16);
> +   }
> +}
Kenneth Graunke <kenneth@whitecape.org> writes:

> On Monday, June 11, 2018 7:25:55 PM PDT Francisco Jerez wrote:
>> This replaces brw_set_message_descriptor() with the composition of
>> brw_set_desc() and a new inline helper function that packs the common
>> message descriptor controls into an integer.  The goal is to represent
>> all message descriptors as a 32-bit integer which is written at once
>> into the instruction, which is more flexible (SENDS anyone?), robust
>> (see d2eecf0b0b24d203d0f171807681dffd830d54de fixing an issue
>> ultimately caused by some bits of the extended message descriptor
>> being left undefined) and future-proof than the current approach of
>> specifying the individual descriptor fields directly into the
>> instruction.
>> 
>> This approach also seems more self-documenting, since it will allow
>> removing calls to functions with way too many arguments like
>> brw_set_*_message() and brw_send_indirect_message(), and instead
>> provide a single descriptor argument constructed from an appropriate
>> combination of brw_*_desc() helpers.
>> 
>> Note that because brw_set_message_descriptor() was (conditionally?)
>> overriding fields of the instruction which strictly speaking weren't
>> part of the message descriptor, this involves calling
>> brw_inst_set_sfid() and brw_inst_set_eot() in some cases in addition
>> to brw_set_desc().
>> ---
>>  src/intel/compiler/brw_eu.h               |  29 +++++---
>>  src/intel/compiler/brw_eu_emit.c          | 108 +++++++++++-------------------
>>  src/intel/compiler/brw_vec4_generator.cpp |  17 +++--
>>  3 files changed, 68 insertions(+), 86 deletions(-)
>> 
>> diff --git a/src/intel/compiler/brw_eu.h b/src/intel/compiler/brw_eu.h
>> index 5a396339fde..b2b20713e45 100644
>> --- a/src/intel/compiler/brw_eu.h
>> +++ b/src/intel/compiler/brw_eu.h
>> @@ -256,14 +256,6 @@ void brw_set_sampler_message(struct brw_codegen *p,
>>                               unsigned simd_mode,
>>                               unsigned return_format);
>>  
>> -void brw_set_message_descriptor(struct brw_codegen *p,
>> -                                brw_inst *inst,
>> -                                enum brw_message_target sfid,
>> -                                unsigned msg_length,
>> -                                unsigned response_length,
>> -                                bool header_present,
>> -                                bool end_of_thread);
>> -
>>  void brw_set_dp_read_message(struct brw_codegen *p,
>>  			     brw_inst *insn,
>>  			     unsigned binding_table_index,
>> @@ -287,6 +279,27 @@ void brw_set_dp_write_message(struct brw_codegen *p,
>>  			      unsigned end_of_thread,
>>  			      unsigned send_commit_msg);
>>  
>> +/**
>> + * Construct a message descriptor immediate with the specified common
>> + * descriptor controls.
>> + */
>> +static inline uint32_t
>> +brw_message_desc(const struct gen_device_info *devinfo,
>> +                 unsigned msg_length,
>> +                 unsigned response_length,
>> +                 bool header_present)
>> +{
>
> Perhaps it would be good to add
>
>       assert(msg_length >= 1 && msg_length <= 15);
>
>> +   if (devinfo->gen >= 5) {
>
>       assert(response_length <= 16);
>
>
>> +      return (msg_length << 25 |
>> +              response_length << 20 |
>> +              header_present << 19);
>> +   } else {
>
>       assert(response_length <= 8);
>
> I'm not so concerned with validating the values here, just thinking it
> might make sense to verify that mlen fits in a U4, for example, so we
> don't accidentally bleed over into other fields when encoding it.
>

It's kind of a PITA to assert that each field is in range manually for
each one of these helpers (the following patches introduce a pile of
functions very much like this one), and verifying that the assertions
are complete and match the definition of the hardware fields is not
straightforward to review.  I would have used the SET_FIELD() macro if
it wasn't because it relies on macros being defined with specific
suffixes for each field.  If you believe it's going to be valuable I
think I'm going to introduce a new helper more easily reusable than
SET_FIELD() checking for overflow based on a bitfield specification, and
use it instead of the plain left-shift operators.

> I suppose the validator already catches this, though...
>

I don't think the validator will be able to catch such cases in general.

>> +      return (msg_length << 20 |
>> +              response_length << 16);
>> +   }
>> +}
On Thursday, June 21, 2018 2:59:30 PM PDT Francisco Jerez wrote:
> Kenneth Graunke <kenneth@whitecape.org> writes:
> 
> > On Monday, June 11, 2018 7:25:55 PM PDT Francisco Jerez wrote:
> >> This replaces brw_set_message_descriptor() with the composition of
> >> brw_set_desc() and a new inline helper function that packs the common
> >> message descriptor controls into an integer.  The goal is to represent
> >> all message descriptors as a 32-bit integer which is written at once
> >> into the instruction, which is more flexible (SENDS anyone?), robust
> >> (see d2eecf0b0b24d203d0f171807681dffd830d54de fixing an issue
> >> ultimately caused by some bits of the extended message descriptor
> >> being left undefined) and future-proof than the current approach of
> >> specifying the individual descriptor fields directly into the
> >> instruction.
> >> 
> >> This approach also seems more self-documenting, since it will allow
> >> removing calls to functions with way too many arguments like
> >> brw_set_*_message() and brw_send_indirect_message(), and instead
> >> provide a single descriptor argument constructed from an appropriate
> >> combination of brw_*_desc() helpers.
> >> 
> >> Note that because brw_set_message_descriptor() was (conditionally?)
> >> overriding fields of the instruction which strictly speaking weren't
> >> part of the message descriptor, this involves calling
> >> brw_inst_set_sfid() and brw_inst_set_eot() in some cases in addition
> >> to brw_set_desc().
> >> ---
> >>  src/intel/compiler/brw_eu.h               |  29 +++++---
> >>  src/intel/compiler/brw_eu_emit.c          | 108 +++++++++++-------------------
> >>  src/intel/compiler/brw_vec4_generator.cpp |  17 +++--
> >>  3 files changed, 68 insertions(+), 86 deletions(-)
> >> 
> >> diff --git a/src/intel/compiler/brw_eu.h b/src/intel/compiler/brw_eu.h
> >> index 5a396339fde..b2b20713e45 100644
> >> --- a/src/intel/compiler/brw_eu.h
> >> +++ b/src/intel/compiler/brw_eu.h
> >> @@ -256,14 +256,6 @@ void brw_set_sampler_message(struct brw_codegen *p,
> >>                               unsigned simd_mode,
> >>                               unsigned return_format);
> >>  
> >> -void brw_set_message_descriptor(struct brw_codegen *p,
> >> -                                brw_inst *inst,
> >> -                                enum brw_message_target sfid,
> >> -                                unsigned msg_length,
> >> -                                unsigned response_length,
> >> -                                bool header_present,
> >> -                                bool end_of_thread);
> >> -
> >>  void brw_set_dp_read_message(struct brw_codegen *p,
> >>  			     brw_inst *insn,
> >>  			     unsigned binding_table_index,
> >> @@ -287,6 +279,27 @@ void brw_set_dp_write_message(struct brw_codegen *p,
> >>  			      unsigned end_of_thread,
> >>  			      unsigned send_commit_msg);
> >>  
> >> +/**
> >> + * Construct a message descriptor immediate with the specified common
> >> + * descriptor controls.
> >> + */
> >> +static inline uint32_t
> >> +brw_message_desc(const struct gen_device_info *devinfo,
> >> +                 unsigned msg_length,
> >> +                 unsigned response_length,
> >> +                 bool header_present)
> >> +{
> >
> > Perhaps it would be good to add
> >
> >       assert(msg_length >= 1 && msg_length <= 15);
> >
> >> +   if (devinfo->gen >= 5) {
> >
> >       assert(response_length <= 16);
> >
> >
> >> +      return (msg_length << 25 |
> >> +              response_length << 20 |
> >> +              header_present << 19);
> >> +   } else {
> >
> >       assert(response_length <= 8);
> >
> > I'm not so concerned with validating the values here, just thinking it
> > might make sense to verify that mlen fits in a U4, for example, so we
> > don't accidentally bleed over into other fields when encoding it.
> >
> 
> It's kind of a PITA to assert that each field is in range manually for
> each one of these helpers (the following patches introduce a pile of
> functions very much like this one), and verifying that the assertions
> are complete and match the definition of the hardware fields is not
> straightforward to review.  I would have used the SET_FIELD() macro if
> it wasn't because it relies on macros being defined with specific
> suffixes for each field.  If you believe it's going to be valuable I
> think I'm going to introduce a new helper more easily reusable than
> SET_FIELD() checking for overflow based on a bitfield specification, and
> use it instead of the plain left-shift operators.

I'm fine with landing things as is, but I do think it would be valuable
to assert that the values are in range.  At the very least, brw_inst has
historically done that, and genxml's similar asserts have caught all
kinds of problems.  I definitely agree that doing this ad-hoc gets
messy, and adding a new macro would help a lot.

You could combine it with the shift, or else just do:

    ASSERT_FITS_IN_BITS(val, bits) assert((val & ~((1 << bits) - 1)) == 0)

and then ASSERT_FITS_IN_BITS(msg_length, 5).  Depends whether you want
to think of it as "fits in bits 64:60" or "is a 5-bit unsigned value".

Up to you :)

> 
> > I suppose the validator already catches this, though...
> >
> 
> I don't think the validator will be able to catch such cases in general.

Right...it should catch things like "mlen 0 is invalid", but
can't/shouldn't check bitwidths for every field.