[Mesa-dev] [PATCH 01/13] i965: Factor out logic to build a send message instruction with indirect descriptor.

[Pohjolainen, Topi]

On Fri, Mar 06, 2015 at 10:37:06AM +0200, Pohjolainen, Topi wrote:
> On Fri, Feb 27, 2015 at 05:34:44PM +0200, Francisco Jerez wrote:
> > ---
> >  src/mesa/drivers/dri/i965/brw_eu.h               | 19 ++++++--
> >  src/mesa/drivers/dri/i965/brw_eu_emit.c          | 58 ++++++++++++++++++------
> >  src/mesa/drivers/dri/i965/brw_fs_generator.cpp   | 55 +++++-----------------
> >  src/mesa/drivers/dri/i965/brw_vec4_generator.cpp | 37 ++++-----------
> >  4 files changed, 77 insertions(+), 92 deletions(-)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/brw_eu.h b/src/mesa/drivers/dri/i965/brw_eu.h
> > index 1b954c8..9b1e0e2 100644
> > --- a/src/mesa/drivers/dri/i965/brw_eu.h
> > +++ b/src/mesa/drivers/dri/i965/brw_eu.h
> > @@ -205,11 +205,6 @@ void brw_set_sampler_message(struct brw_compile *p,
> >                               unsigned simd_mode,
> >                               unsigned return_format);
> >  
> > -void brw_set_indirect_send_descriptor(struct brw_compile *p,
> > -                                      brw_inst *insn,
> > -                                      unsigned sfid,
> > -                                      struct brw_reg descriptor);
> > -
> >  void brw_set_dp_read_message(struct brw_compile *p,
> >  			     brw_inst *insn,
> >  			     unsigned binding_table_index,
> > @@ -243,6 +238,20 @@ void brw_urb_WRITE(struct brw_compile *p,
> >  		   unsigned offset,
> >  		   unsigned swizzle);
> >  
> > +/**
> > + * Send message to shared unit \p sfid with a possibly indirect descriptor \p
> > + * desc.  If the descriptor is not an immediate it will be transparently
> > + * loaded to an address register using an OR instruction that will be returned
> > + * to the caller so additional descriptor bits can be specified with the usual
> > + * brw_set_*_message() helper functions.
> > + */

Right, you exploit this in patch number five. I think at least this comment
is misleading as it doesn't say anything about the returned instruction
in case the given descriptor is an immediate.

All in all I'm not too happy about the return value having such differing
semantics depending on the given descriptor type.

> > +struct brw_inst *
> > +brw_send_indirect_message(struct brw_compile *p,
> > +                          unsigned sfid,
> > +                          struct brw_reg dst,
> > +                          struct brw_reg payload,
> > +                          struct brw_reg desc);
> > +
> >  void brw_ff_sync(struct brw_compile *p,
> >  		   struct brw_reg dest,
> >  		   unsigned msg_reg_nr,
> > diff --git a/src/mesa/drivers/dri/i965/brw_eu_emit.c b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> > index e69840a..cd2ce92 100644
> > --- a/src/mesa/drivers/dri/i965/brw_eu_emit.c
> > +++ b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> > @@ -751,21 +751,6 @@ brw_set_sampler_message(struct brw_compile *p,
> >     }
> >  }
> >  
> > -void brw_set_indirect_send_descriptor(struct brw_compile *p,
> > -                                      brw_inst *insn,
> > -                                      unsigned sfid,
> > -                                      struct brw_reg descriptor)
> > -{
> > -   /* Only a0.0 may be used as SEND's descriptor operand. */
> > -   assert(descriptor.file == BRW_ARCHITECTURE_REGISTER_FILE);
> > -   assert(descriptor.type == BRW_REGISTER_TYPE_UD);
> > -   assert(descriptor.nr == BRW_ARF_ADDRESS);
> > -   assert(descriptor.subnr == 0);
> > -
> > -   brw_set_message_descriptor(p, insn, sfid, 0, 0, false, false);
> > -   brw_set_src1(p, insn, descriptor);
> > -}
> > -
> >  static void
> >  gen7_set_dp_scratch_message(struct brw_compile *p,
> >                              brw_inst *inst,
> > @@ -2490,6 +2475,49 @@ void brw_urb_WRITE(struct brw_compile *p,
> >  		       swizzle);
> >  }
> >  
> > +struct brw_inst *
> > +brw_send_indirect_message(struct brw_compile *p,
> > +                          unsigned sfid,
> > +                          struct brw_reg dst,
> > +                          struct brw_reg payload,
> > +                          struct brw_reg desc)
> > +{
> > +   const struct brw_context *brw = p->brw;
> > +   struct brw_inst *send, *setup;
> > +
> > +   assert(desc.type == BRW_REGISTER_TYPE_UD);
> > +
> > +   if (desc.file == BRW_IMMEDIATE_VALUE) {
> > +      setup = send = next_insn(p, BRW_OPCODE_SEND);
> 
> If I'm reading this correctly, all the callers in this patch use 'desc' of
> type other than BRW_IMMEDIATE_VALUE. Hence returning the actual
> send-instruction as the descriptor instuction is not needed by any of the
> logic modified in this patch. Do we really need to do this or could we just
> return NULL since in this case there really isn't any OR-instruction setting
> the descriptor bits? (Your documentation above says that the returned
> instruction is an OR setting the descriptor. Returning the SEND instead is
> not the same really).