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