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();
>
Right. Good point. I'll do it that way instead.
>
> 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.
>
Perhaps. Honestly, I'm not sure why CSE isn't causing problems now unless
it doesn't do CSE on message registers.
--Jason
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
