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

[Pohjolainen, Topi]

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").

> +      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.

Given that this is how it works:

Reviewed-by: Topi Pohjolainen <topi.pohjolainen at intel.com>

> +
> +      if (earlier_block->can_combine_with(jump_block)) {
> +         earlier_block->combine_with(jump_block);
> +
> +         block = earlier_block;
> +      }
>  
>        progress = true;
>     }
>  
>     if (progress)
> -      invalidate_live_intervals();
> +      invalidate_live_intervals(false);
>  
>     return progress;
>  }
> -- 
> 1.8.5.5
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev