[Mesa-dev] [PATCH] i965/vec4: Consider sources of non-GRF-dst instructions for dead channels.

[Matt Turner]

On Mon, Mar 31, 2014 at 10:44 AM, Eric Anholt <eric at anholt.net> wrote:
> Matt Turner <mattst88 at gmail.com> writes:
>
>> Previously we'd ignore the sources of instructions with non-GRF
>> destinations when calculating calculating the dead channels. This would
>> lead to us incorrectly removing the first instruction in this sequence:
>>
>>    mov vgrf11, ...
>>    cmp.ne.f0 null, vgrf11, 1.0
>>    mov vgrf11, ...
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76616
>> ---
>>  src/mesa/drivers/dri/i965/brw_vec4.cpp | 22 ++++++++++------------
>>  1 file changed, 10 insertions(+), 12 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_vec4.cpp b/src/mesa/drivers/dri/i965/brw_vec4.cpp
>> index 32a3892..4d8740a 100644
>> --- a/src/mesa/drivers/dri/i965/brw_vec4.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_vec4.cpp
>> @@ -324,6 +324,9 @@ src_reg::equals(src_reg *r)
>>  static bool
>>  try_eliminate_instruction(vec4_instruction *inst, int new_writemask)
>>  {
>> +   if (inst->has_side_effects())
>> +      return false;
>> +
>>     if (new_writemask == 0) {
>>        /* Don't dead code eliminate instructions that write to the
>>         * accumulator as a side-effect. Instead just set the destination
>> @@ -383,9 +386,6 @@ vec4_visitor::dead_code_eliminate()
>>
>>        seen_control_flow = inst->is_control_flow() || seen_control_flow;
>>
>> -      if (inst->has_side_effects())
>> -         continue;
>> -
>>        bool inst_writes_flag = false;
>>        if (inst->dst.file != GRF) {
>>           if (inst->dst.is_null() && inst->writes_flag()) {
>> @@ -438,22 +438,19 @@ vec4_visitor::dead_code_eliminate()
>>             node = prev, prev = prev->prev) {
>>           vec4_instruction *scan_inst = (vec4_instruction  *)node;
>>
>> -         if (scan_inst->has_side_effects())
>> -            continue;
>> -
>>           if (inst_writes_flag) {
>>              if (scan_inst->dst.is_null() && scan_inst->writes_flag()) {
>>                 scan_inst->remove();
>>                 progress = true;
>> +               continue;
>
> You don't want to be remove()ing in the has_side_effects() case, right?
> (think removing an atomic inc).

I moved the has_side_effects() check into try_eliminate_instruction(),
since we want to consider sources of instructions with side-effects
when determining which channels are alive in our backwards walk.

>> -         } else if (scan_inst->dst.file != GRF) {
>> -            continue;
>>           }
>>
>> -         if (inst->dst.reg == scan_inst->dst.reg) {
>> +         if (inst->dst.file == GRF &&
>> +             scan_inst->dst.file == GRF &&
>> +             inst->dst.reg == scan_inst->dst.reg) {
>>              int new_writemask = scan_inst->dst.writemask & ~dead_channels;
>>
>>              progress = try_eliminate_instruction(scan_inst, new_writemask) ||
>
> Doesn't this change prevent the inst_writes_flag == true case from
> optimizing?

I thought so after reading your reply, but upon further investigation,
I think not. We certainly take a hit in shader-db from this patch, but
I'm not convinced that it's not all from bad optimization before.

The block immediately above this hunk is

 if (inst_writes_flag) {
    if (scan_inst->dst.is_null() && scan_inst->writes_flag()) {
       scan_inst->remove();
       progress = true;
       continue;
    } else if (scan_inst->reads_flag()) {
       break;
    }
 }

so I think we're handling it properly. I picked out a shader that was
hurt by this patch (l4d2/1606.shader_test) and we were definitely
optimizing it incorrectly before.

I did make a small change to the last two hunks of the patch though,
so I'll resend.