[Mesa-dev] [PATCH 18/22] intel: Use TXQ for image_size when we have a typed surface

[Kenneth Graunke]

On Wednesday, August 29, 2018 5:38:59 AM PDT Jason Ekstrand wrote:
> On Wed, Aug 29, 2018 at 1:36 AM Kenneth Graunke <kenneth at whitecape.org>
> wrote:
> 
> > On Friday, August 17, 2018 1:06:24 PM PDT Jason Ekstrand wrote:
> > > ---
> > >  src/intel/compiler/brw_eu_defines.h           |  3 +++
> > >  src/intel/compiler/brw_fs.cpp                 |  8 ++++++
> > >  src/intel/compiler/brw_fs_generator.cpp       | 26 ++++++++++++++++---
> > >  src/intel/compiler/brw_fs_nir.cpp             | 26 +++++++++++++++++++
> > >  .../compiler/brw_nir_lower_image_load_store.c | 15 +++++++++++
> > >  src/intel/compiler/brw_shader.cpp             |  5 ++++
> > >  6 files changed, 79 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/src/intel/compiler/brw_eu_defines.h
> > b/src/intel/compiler/brw_eu_defines.h
> > > index 2289fc9be70..c36699d7676 100644
> > > --- a/src/intel/compiler/brw_eu_defines.h
> > > +++ b/src/intel/compiler/brw_eu_defines.h
> > > @@ -354,6 +354,9 @@ enum opcode {
> > >     SHADER_OPCODE_SAMPLEINFO,
> > >     SHADER_OPCODE_SAMPLEINFO_LOGICAL,
> > >
> > > +   SHADER_OPCODE_IMAGE_SIZE,
> > > +   SHADER_OPCODE_IMAGE_SIZE_LOGICAL,
> > > +
> > >     /**
> > >      * Combines multiple sources of size 1 into a larger virtual GRF.
> > >      * For example, parameters for a send-from-GRF message.  Or, updating
> > > diff --git a/src/intel/compiler/brw_fs.cpp
> > b/src/intel/compiler/brw_fs.cpp
> > > index 5b87991652d..1e1954270e8 100644
> > > --- a/src/intel/compiler/brw_fs.cpp
> > > +++ b/src/intel/compiler/brw_fs.cpp
> > > @@ -4946,6 +4946,14 @@ fs_visitor::lower_logical_sends()
> > >           lower_sampler_logical_send(ibld, inst,
> > SHADER_OPCODE_SAMPLEINFO);
> > >           break;
> > >
> > > +      case SHADER_OPCODE_IMAGE_SIZE_LOGICAL:
> > > +         /* Nothing needs to be done except changing the opcode and
> > setting a
> > > +          * message length.
> > > +          */
> > > +         inst->opcode = SHADER_OPCODE_IMAGE_SIZE;
> > > +         inst->mlen = inst->exec_size / 8;
> >
> > I'm not sure it's even worth having a LOGICAL opcode for this...
> > maybe just set the message length when creating it and call it a day?
> >
> 
> The problem is that it has to go through SIMD width lowering which doesn't
> understand mlen.  We could make SIMD width lowering just shrink the mlen
> but that's wrong for almost all opcodes.  Thoughts?

Hmm.  SIMD width lowering seems a bit odd, given that the parameter is
always a single immediate (LOD 0), and the result is the same for all
channels (the size is the size).  Couldn't you just do it in SIMD8,
and have MOVs that expand it?

If we do need to go through SIMD-width lowering, then I agree, setting
mlen at the end is probably best...

> > > +         break;
> > > +
> > >        case SHADER_OPCODE_UNTYPED_SURFACE_READ_LOGICAL:
> > >           lower_surface_logical_send(ibld, inst,
> > >                                      SHADER_OPCODE_UNTYPED_SURFACE_READ,
> > > diff --git a/src/intel/compiler/brw_fs_generator.cpp
> > b/src/intel/compiler/brw_fs_generator.cpp
> > > index e265d59ccbe..13293d7efbd 100644
> > > --- a/src/intel/compiler/brw_fs_generator.cpp
> > > +++ b/src/intel/compiler/brw_fs_generator.cpp
> > > @@ -1017,6 +1017,9 @@ fs_generator::generate_tex(fs_inst *inst, struct
> > brw_reg dst, struct brw_reg src
> > >        case SHADER_OPCODE_SAMPLEINFO:
> > >           msg_type = GEN6_SAMPLER_MESSAGE_SAMPLE_SAMPLEINFO;
> > >           break;
> > > +      case SHADER_OPCODE_IMAGE_SIZE:
> > > +      msg_type = GEN5_SAMPLER_MESSAGE_SAMPLE_RESINFO;
> > > +      break;
> >
> > Indentation here seems off.
> >
> 
> Wow.  Somehow I got tabs in there.  My .vimrc should make that almost
> impossible...
> 
> 
> > Maybe just add this case label above the
> > existing case SHADER_OPCODE_TXS, which does the exact same thing?
> >
> 
> Sure, why not.
> 
> 
> > >        default:
> > >        unreachable("not reached");
> > >        }
> > > @@ -1126,10 +1129,20 @@ fs_generator::generate_tex(fs_inst *inst, struct
> > brw_reg dst, struct brw_reg src
> > >        }
> > >     }
> > >
> > > -   uint32_t base_binding_table_index = (inst->opcode ==
> > SHADER_OPCODE_TG4 ||
> > > -         inst->opcode == SHADER_OPCODE_TG4_OFFSET)
> > > -         ? prog_data->binding_table.gather_texture_start
> > > -         : prog_data->binding_table.texture_start;
> > > +   uint32_t base_binding_table_index;
> > > +   switch (inst->opcode) {
> > > +   case SHADER_OPCODE_TG4:
> > > +   case SHADER_OPCODE_TG4_OFFSET:
> > > +      base_binding_table_index =
> > prog_data->binding_table.gather_texture_start;
> > > +      break;
> > > +   case SHADER_OPCODE_IMAGE_SIZE:
> > > +      /* Image binding table indices are absolute */
> >
> > Uh, what?  That doesn't seem right.  Shouldn't this be
> >
> >       base_binding_table_index = prog_data->binding_table.image_start;
> >
> 
> No... It's dumb.  The binding table index that gets placed in the uniform
> image param is an absolute binding table index.  If I move this patch after
> the second-to-last patch, we should be able to avoid this mess.  Why don't
> I try to do that.

Oh, I see, I entirely missed how BTI was getting passed in...it is
indeed coming from the uniform.  So this does seem right.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180829/5f4e9aae/attachment.sig>