[Mesa-dev] [PATCH] i965/cse: Don't eliminate instructions with side-effects

[Kenneth Graunke]

On Friday, August 08, 2014 02:42:27 PM Jason Ekstrand wrote:
> In particular, this caused problems where atomics operations were getting
> eliminated.
> 
> Signed-off-by: Jason Ekstrand <jason.ekstrand at intel.com>
> ---
>  src/mesa/drivers/dri/i965/brw_fs_cse.cpp   | 3 ++-
>  src/mesa/drivers/dri/i965/brw_vec4_cse.cpp | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_cse.cpp b/src/mesa/drivers/dri/i965/brw_fs_cse.cpp
> index 63d87f9..8cfc6c6 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_cse.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_cse.cpp
> @@ -177,7 +177,8 @@ fs_visitor::opt_cse_local(bblock_t *block)
>     foreach_inst_in_block(fs_inst, inst, block) {
>        /* Skip some cases. */
>        if (is_expression(inst) && !inst->is_partial_write() &&
> -          (inst->dst.file != HW_REG || inst->dst.is_null()))
> +          (inst->dst.file != HW_REG || inst->dst.is_null()) &&
> +          !inst->has_side_effects())
>        {
>           bool found = false;
>  
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_cse.cpp b/src/mesa/drivers/dri/i965/brw_vec4_cse.cpp
> index 29d2e02..44651b4 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_cse.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_cse.cpp
> @@ -135,7 +135,8 @@ vec4_visitor::opt_cse_local(bblock_t *block)
>     foreach_inst_in_block (vec4_instruction, inst, block) {
>        /* Skip some cases. */
>        if (is_expression(inst) && !inst->predicate && inst->mlen == 0 &&
> -          (inst->dst.file != HW_REG || inst->dst.is_null()))
> +          (inst->dst.file != HW_REG || inst->dst.is_null()) &&
> +          !inst->has_side_effects())
>        {
>           bool found = false;
>  
> 

I was confused at first because operations with side-effects should never have been part of the whitelist of opcodes to CSE.  But Matt generalized it in 1d97212007ccae, by changing is_expression()'s default case to "return inst->is_send_from_grf()".

I think a better patch would be to change that to:

   default:
      return inst->is_send_from_grf() && !inst->has_side_effects();

It's also worth noting in your commit message that this is not actually fixing a current bug, but rather preventing a regression once your patches that convert atomics to send-from-GRFs land.

--Ken
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140808/a982692e/attachment.sig>