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

[Jordan Justen]

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.

r-b?

-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;
> 
>