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

[Jordan Justen]

On 2015-10-22 00:06:37, Iago Toral wrote:
> 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 
> 
> 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.

Yeah. I only saw this after applying some optimization patches from krh.

-Jordan
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [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 

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


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [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 
> > ---
> >  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;
> 
> 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Jordan Justen]

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)

Signed-off-by: Jordan Justen 
---
 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;
-- 
2.5.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Iago Toral]

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.

Iago

> Signed-off-by: Jordan Justen 
> ---
>  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;


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev