[Mesa-dev] [PATCH] i965/fs: Disable CSE optimization for untyped & typed surface reads

[Iago Toral]

On Wed, 2015-10-21 at 23:24 -0700, Jordan Justen wrote:
> On 2015-10-20 00:43:13, Iago Toral wrote:
> > On Tue, 2015-10-20 at 00:12 -0700, Jordan Justen wrote:
> > > An untyped surface read is volatile because it might be affected by a
> > > write.
> > > 
> > > In the ES31-CTS.compute_shader.resources-max test, two back to back
> > > read/modify/writes of an SSBO variable looked something like this:
> > > 
> > >   r1 = untyped_surface_read(ssbo_float)
> > >   r2 = r1 + 1
> > >   untyped_surface_write(ssbo_float, r2)
> > >   r3 = untyped_surface_read(ssbo_float)
> > >   r4 = r3 + 1
> > >   untyped_surface_write(ssbo_float, r4)
> > > 
> > > And after CSE, we had:
> > > 
> > >   r1 = untyped_surface_read(ssbo_float)
> > >   r2 = r1 + 1
> > >   untyped_surface_write(ssbo_float, r2)
> > >   r4 = r1 + 1
> > >   untyped_surface_write(ssbo_float, r4)
> > 
> > Yeah, we cannot do CSE with SSBO loads. Patch looks good to me, but we
> > should do the same in the vec4 CSE pass.
> 
> Yeah, I checked vec4 CSE. It looks like is_expression will
> unconditionally return false for those opcodes.

Oh right.

> r-b?

Reviewed-by: Iago Toral Quiroga <itoral at igalia.com>

FWIW, my ssbo load optimization pass is trying to "undo" this since it
is all about doing CSE for ssbo loads that are safe to CSE, that is,
when we know that we don't have stores/atomics that write to the same
offset or memory barriers in between. I am trying to implement that in
NIR though, so we still need this, to prevent i965 from trying to CSE
the remaining loads it sees, since thise would not be safe to CSE.

Also, as I mentioned in another e-mail, we did not notice this issue
earlier was because there are a couple of problems in i965 that make it
quite difficult that the CSE pass identifies identical SSBO loads at the
moment, but that is bound to change as soon as those things get
eventually fixed.

Iago

> -Jordan
> 
> > 
> > > Signed-off-by: Jordan Justen <jordan.l.justen at intel.com>
> > > ---
> > >  src/mesa/drivers/dri/i965/brw_fs_cse.cpp |  3 ++-
> > >  src/mesa/drivers/dri/i965/brw_shader.cpp | 14 ++++++++++++++
> > >  src/mesa/drivers/dri/i965/brw_shader.h   |  6 ++++++
> > >  3 files changed, 22 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/mesa/drivers/dri/i965/brw_fs_cse.cpp b/src/mesa/drivers/dri/i965/brw_fs_cse.cpp
> > > index c7628dc..3a28c8d 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_fs_cse.cpp
> > > +++ b/src/mesa/drivers/dri/i965/brw_fs_cse.cpp
> > > @@ -93,7 +93,8 @@ is_expression(const fs_visitor *v, const fs_inst *const inst)
> > >     case SHADER_OPCODE_LOAD_PAYLOAD:
> > >        return !inst->is_copy_payload(v->alloc);
> > >     default:
> > > -      return inst->is_send_from_grf() && !inst->has_side_effects();
> > > +      return inst->is_send_from_grf() && !inst->has_side_effects() &&
> > > +         !inst->is_volatile();
> > >     }
> > >  }
> > >  
> > > diff --git a/src/mesa/drivers/dri/i965/brw_shader.cpp b/src/mesa/drivers/dri/i965/brw_shader.cpp
> > > index 2324b56..be911ed 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_shader.cpp
> > > +++ b/src/mesa/drivers/dri/i965/brw_shader.cpp
> > > @@ -969,6 +969,20 @@ backend_instruction::has_side_effects() const
> > >     }
> > >  }
> > >  
> > > +bool
> > > +backend_instruction::is_volatile() const
> > > +{
> > > +   switch (opcode) {
> > > +   case SHADER_OPCODE_UNTYPED_SURFACE_READ:
> > > +   case SHADER_OPCODE_UNTYPED_SURFACE_READ_LOGICAL:
> > > +   case SHADER_OPCODE_TYPED_SURFACE_READ:
> > > +   case SHADER_OPCODE_TYPED_SURFACE_READ_LOGICAL:
> > > +      return true;
> > > +   default:
> > > +      return false;
> > > +   }
> > > +}
> > > +
> > >  #ifndef NDEBUG
> > >  static bool
> > >  inst_is_in_block(const bblock_t *block, const backend_instruction *inst)
> > > diff --git a/src/mesa/drivers/dri/i965/brw_shader.h b/src/mesa/drivers/dri/i965/brw_shader.h
> > > index b33b08f..35ee210 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_shader.h
> > > +++ b/src/mesa/drivers/dri/i965/brw_shader.h
> > > @@ -115,6 +115,12 @@ struct backend_instruction : public exec_node {
> > >      * optimize these out unless you know what you are doing.
> > >      */
> > >     bool has_side_effects() const;
> > > +
> > > +   /**
> > > +    * True if the instruction might be affected by side effects of other
> > > +    * instructions.
> > > +    */
> > > +   bool is_volatile() const;
> > >  #else
> > >  struct backend_instruction {
> > >     struct exec_node link;
> > 
> > 
>