[Mesa-dev] [PATCH] i965: Don't mark dead instructions' sources live.

[Matt Turner]

On Fri, Nov 27, 2015 at 7:32 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On Wednesday, November 25, 2015 06:14:28 PM Matt Turner wrote:
>> Removes dead code from glsl-mat-from-int-ctor-03.shader_test.
>>
>> Reported-by: Juan A. Suarez Romero <jasuarez at igalia.com>
>> ---
>>  src/mesa/drivers/dri/i965/brw_fs_dead_code_eliminate.cpp   | 4 ++++
>>  src/mesa/drivers/dri/i965/brw_vec4_dead_code_eliminate.cpp | 4 ++++
>>  2 files changed, 8 insertions(+)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_dead_code_eliminate.cpp b/src/mesa/drivers/dri/i965/brw_fs_dead_code_eliminate.cpp
>> index a50cf6f..6b4b602 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs_dead_code_eliminate.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_dead_code_eliminate.cpp
>> @@ -109,6 +109,10 @@ fs_visitor::dead_code_eliminate()
>>              BITSET_CLEAR(flag_live, inst->flag_subreg);
>>           }
>>
>> +         /* Don't mark dead instructions' sources as live */
>> +         if (inst->opcode == BRW_OPCODE_NOP)
>> +            continue;
>> +
>>           for (int i = 0; i < inst->sources; i++) {
>>              if (inst->src[i].file == VGRF) {
>>                 int var = live_intervals->var_from_reg(inst->src[i]);
>> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_dead_code_eliminate.cpp b/src/mesa/drivers/dri/i965/brw_vec4_dead_code_eliminate.cpp
>> index 58aed81..369941b 100644
>> --- a/src/mesa/drivers/dri/i965/brw_vec4_dead_code_eliminate.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_vec4_dead_code_eliminate.cpp
>> @@ -150,6 +150,10 @@ vec4_visitor::dead_code_eliminate()
>>                 BITSET_CLEAR(flag_live, c);
>>           }
>>
>> +         /* Don't mark dead instructions' sources as live */
>> +         if (inst->opcode == BRW_OPCODE_NOP)
>> +            continue;
>> +
>>           for (int i = 0; i < 3; i++) {
>>              if (inst->src[i].file == VGRF) {
>>                 for (unsigned j = 0; j < inst->regs_read(i); j++) {
>>
>
> This is
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

Thanks!

> A while back I grumbled at you and Tapani about not updating
> inst->sources and leaving junk source registers in place when mutating
> an instruction to change its opcode.

I suspect you're thinking about commit bb33a31c and Tapani's fix
commit 627c6830. The bug Tapani's commit fixed had nothing to do with
inst->sources or left behind junk in the src[] array. I can't find
where you said anything on list, but I have a memory of talking about
it in person.

The story is, none of the opt_algebraic code (except Curro's more
recent SHADER_OPCODE_BROADCAST additions) modifies inst->sources --
most of the code predates LOAD_PAYLOAD (the reason inst->sources
exists). When I made commit bb33a31c, I just kept doing what the
existing code does: set unused sources to reg_undef.

> If inst->sources were updated correctly when converting to NOP (i.e. set
> to 0), you would not have this bug, as the loop over sources would not
> happen.  You wouldn't need to special case NOP here, either.

That's not true.

You could set inst->sources to 0, but you'd also unset inst->predicate
so that reads_flag() doesn't return true. And, you'd have to figure
out something different to do in the vec4 backend, because it doesn't
have inst->sources. Also, there are three places in the DCE where we
decide an instruction is dead, so you'd have to do those steps in
three different places, while *still* setting opcode = NOP.

Or, you can just look for opcode == NOP, which I think is a much
better solution especially since it's what you have to do in order to
find and delete the instruction later anyway. opcode == NOP is *the*
uniquely identifying piece of information to determine if the
instruction is dead.

> Which actually makes me wonder: if we convert from ADD to MOV (or
> similar), do we leave a bogus src[1] in place and treat it as live?
> Presumably most cases where that happens are because we did algebraic
> optimizations involving IMMs, so the extra live thing would be an
> immediate and not cause any trouble...

No, we set dead sources to reg_undef in opt_algebraic.