On Fri, Aug 8, 2014 at 4:58 PM, Kenneth Graunke <kenn...@whitecape.org> wrote: > 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.ekstr...@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();
Yeah, that seems fine assuming we never add a non-send-from-grf instruction to the has_side_effect list. I think that's a safe assumption, since the other instructions that have "side effects" are things that e.g., implicitly write the accumulator, i.e., things we can still track. Either way works for me. Reviewed-by: Matt Turner <matts...@gmail.com> _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev