On Wed, Jan 8, 2014 at 1:00 PM, Kenneth Graunke <kenn...@whitecape.org> wrote:
> On 01/08/2014 12:43 PM, Matt Turner wrote:
>> total instructions in shared programs: 1500212 -> 1500153 (-0.00%)
>> instructions in affected programs: 17777 -> 17718 (-0.33%)
>> ---
>> src/mesa/drivers/dri/i965/brw_dead_control_flow.cpp | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_dead_control_flow.cpp
>> b/src/mesa/drivers/dri/i965/brw_dead_control_flow.cpp
>> index 63a3e5b..82e83b8 100644
>> --- a/src/mesa/drivers/dri/i965/brw_dead_control_flow.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_dead_control_flow.cpp
>> @@ -33,6 +33,7 @@
>> *
>> * - if/endif
>> * - if/else/endif
>> + * - .../else/endif
>
> Presumably the point of this is to change:
> IF, foo, bar, ELSE, ENDIF ===> IF, foo, bar, ENDIF
>
> ...but if I can read, that's not what this does at all...
>
>> */
>> bool
>> dead_control_flow_eliminate(backend_visitor *v)
>> @@ -59,16 +60,17 @@ dead_control_flow_eliminate(backend_visitor *v)
>> found = true;
>> } else if (prev_inst->opcode == BRW_OPCODE_ELSE) {
>> else_inst = prev_inst;
>> + found = true;
>>
>> prev_inst = (backend_instruction *) prev_inst->prev;
>> if (prev_inst->opcode == BRW_OPCODE_IF) {
>> if_inst = prev_inst;
>> - found = true;
>> }
>> }
>>
>> if (found) {
>> - if_inst->remove();
>> + if (if_inst)
>> + if_inst->remove();
>> if (else_inst)
>> else_inst->remove();
>> endif_inst->remove();
>>
>
> So in the above case...found = true, endif_inst != NULL, else_inst !=
> NULL, but if_inst == NULL.
>
> You delete ELSE, and you delete ENDIF. You leave IF in place...OUCH.
>
> I think you could fix this by changing this to:
>
> if (else_inst)
> else_inst->remove();
>
> if (if_inst) {
> if_inst->remove();
> endif_inst->remove();
> }
Yep, that does seem bogus. I'll send an updated patch.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
