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

[Matt Turner]

On Fri, Aug 8, 2014 at 4:58 PM, Kenneth Graunke <kenneth at 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.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();

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 <mattst88 at gmail.com>