On Fri, Nov 27, 2015 at 7:32 PM, Kenneth Graunke <kenn...@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 <jasua...@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 <kenn...@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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
