<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Aug 8, 2014 at 4:58 PM, Kenneth Graunke <span dir="ltr"><<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Friday, August 08, 2014 02:42:27 PM Jason Ekstrand wrote:<br>
> In particular, this caused problems where atomics operations were getting<br>
> eliminated.<br>
><br>
> Signed-off-by: Jason Ekstrand <<a href="mailto:jason.ekstrand@intel.com">jason.ekstrand@intel.com</a>><br>
> ---<br>
>  src/mesa/drivers/dri/i965/brw_fs_cse.cpp   | 3 ++-<br>
>  src/mesa/drivers/dri/i965/brw_vec4_cse.cpp | 3 ++-<br>
>  2 files changed, 4 insertions(+), 2 deletions(-)<br>
><br>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_cse.cpp b/src/mesa/drivers/dri/i965/brw_fs_cse.cpp<br>
> index 63d87f9..8cfc6c6 100644<br>
> --- a/src/mesa/drivers/dri/i965/brw_fs_cse.cpp<br>
> +++ b/src/mesa/drivers/dri/i965/brw_fs_cse.cpp<br>
> @@ -177,7 +177,8 @@ fs_visitor::opt_cse_local(bblock_t *block)<br>
>     foreach_inst_in_block(fs_inst, inst, block) {<br>
>        /* Skip some cases. */<br>
>        if (is_expression(inst) && !inst->is_partial_write() &&<br>
> -          (inst->dst.file != HW_REG || inst->dst.is_null()))<br>
> +          (inst->dst.file != HW_REG || inst->dst.is_null()) &&<br>
> +          !inst->has_side_effects())<br>
>        {<br>
>           bool found = false;<br>
><br>
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_cse.cpp b/src/mesa/drivers/dri/i965/brw_vec4_cse.cpp<br>
> index 29d2e02..44651b4 100644<br>
> --- a/src/mesa/drivers/dri/i965/brw_vec4_cse.cpp<br>
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_cse.cpp<br>
> @@ -135,7 +135,8 @@ vec4_visitor::opt_cse_local(bblock_t *block)<br>
>     foreach_inst_in_block (vec4_instruction, inst, block) {<br>
>        /* Skip some cases. */<br>
>        if (is_expression(inst) && !inst->predicate && inst->mlen == 0 &&<br>
> -          (inst->dst.file != HW_REG || inst->dst.is_null()))<br>
> +          (inst->dst.file != HW_REG || inst->dst.is_null()) &&<br>
> +          !inst->has_side_effects())<br>
>        {<br>
>           bool found = false;<br>
><br>
><br>
<br>
</div></div>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()". <br>
</blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
I think a better patch would be to change that to:<br>
<br>
   default:<br>
      return inst->is_send_from_grf() && !inst->has_side_effects();<br></blockquote><div><br></div><div>Right. Good point.  I'll do it that way instead.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<br>
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.<br></blockquote><div><br>
</div><div>Perhaps.  Honestly, I'm not sure why CSE isn't causing problems now unless it doesn't do CSE on message registers.<br></div><div><br></div><div>--Jason<br></div></div></div></div>