[Mesa-dev] [PATCH 20/20] i965/fs: Preserve CFG in predicated break pass.

[Matt Turner]

On Tue, Aug 19, 2014 at 1:52 AM, Pohjolainen, Topi
<topi.pohjolainen at intel.com> wrote:
> On Thu, Jul 24, 2014 at 07:54:27PM -0700, Matt Turner wrote:
>> Operating on this code,
>>
>> B0: ...
>>     cmp.ne.f0(8)
>>     (+f0) if(8)
>> B1: break(8)
>> B2: endif(8)
>>
>> We can delete B2 without attempting to merge any blocks, since the
>> break/continue instruction necessarily ends the previous block.
>>
>> After deleting the if instruction, we attempt to merge blocks B0 and B1.
>> ---
>>  .../dri/i965/brw_fs_peephole_predicated_break.cpp  | 29 +++++++++++++++++++---
>>  1 file changed, 25 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_peephole_predicated_break.cpp b/src/mesa/drivers/dri/i965/brw_fs_peephole_predicated_break.cpp
>> index 445d10e..ab197ee 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs_peephole_predicated_break.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_peephole_predicated_break.cpp
>> @@ -64,27 +64,48 @@ fs_visitor::opt_peephole_predicated_break()
>>        if (endif_inst->opcode != BRW_OPCODE_ENDIF)
>>           continue;
>>
>> +      bblock_t *jump_block = block;
>> +      bblock_t *if_block = (bblock_t *)jump_block->link.prev;
>> +      bblock_t *endif_block = (bblock_t *)jump_block->link.next;
>> +
>>        /* For Sandybridge with IF with embedded comparison we need to emit an
>>         * instruction to set the flag register.
>>         */
>>        if (brw->gen == 6 && if_inst->conditional_mod) {
>>           fs_inst *cmp_inst = CMP(reg_null_d, if_inst->src[0], if_inst->src[1],
>>                                   if_inst->conditional_mod);
>> -         if_inst->insert_before(cmp_inst);
>> +         if_inst->insert_before(if_block, cmp_inst);
>>           jump_inst->predicate = BRW_PREDICATE_NORMAL;
>>        } else {
>>           jump_inst->predicate = if_inst->predicate;
>>           jump_inst->predicate_inverse = if_inst->predicate_inverse;
>>        }
>>
>> -      if_inst->remove();
>> -      endif_inst->remove();
>> +      bblock_t *earlier_block = if_block;
>> +      if (if_block->start_ip == if_block->end_ip) {
>> +         earlier_block = (bblock_t *)if_block->link.prev;
>> +      }
>> +
>> +      if_inst->remove(if_block);
>> +      endif_inst->remove(endif_block);
>> +
>> +      if_block->children.make_empty();
>> +      endif_block->parents.make_empty();
>> +
>> +      if_block->add_successor(cfg->mem_ctx, jump_block);
>
> I'm just checking how well I understood things. As we don't have a method
> remove_successor() we need to call make_empty() and then add jump_block
> again as a successor. It was already a successor before we called make_empty(),
> right? (Above we have "if_block = (bblock_t *)jump_block->link.prev").

Yes, exactly. I went back and forth about how to do this.

>> +      jump_block->add_successor(cfg->mem_ctx, endif_block);
>
> And the same thing applies here, we called make_empty() to clear the parent
> list of endif_block so we need to associate jump_block and endif_block again.

Yep. Thanks for the review!