[Mesa-dev] [PATCH 09/20] i965/cfg: Add a function to remove a block from the cfg.

[Pohjolainen, Topi]

On Thu, Jul 24, 2014 at 07:54:16PM -0700, Matt Turner wrote:
> ---
>  src/mesa/drivers/dri/i965/brw_cfg.cpp | 58 +++++++++++++++++++++++++++++++++--
>  src/mesa/drivers/dri/i965/brw_cfg.h   |  4 ++-
>  2 files changed, 58 insertions(+), 4 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_cfg.cpp b/src/mesa/drivers/dri/i965/brw_cfg.cpp
> index 9cd8b9f..c39edad 100644
> --- a/src/mesa/drivers/dri/i965/brw_cfg.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_cfg.cpp
> @@ -50,8 +50,8 @@ link(void *mem_ctx, bblock_t *block)
>     return &l->link;
>  }
>  
> -bblock_t::bblock_t() :
> -   start_ip(0), end_ip(0), num(0)
> +bblock_t::bblock_t(cfg_t *cfg) :
> +   cfg(cfg), start_ip(0), end_ip(0), num(0)

Member cfg does not exist before patch number 10.

>  {
>     start = NULL;
>     end = NULL;
> @@ -291,10 +291,62 @@ cfg_t::~cfg_t()
>     ralloc_free(mem_ctx);
>  }
>  
> +void
> +cfg_t::remove_block(bblock_t *block)
> +{
> +   foreach_list_typed_safe (bblock_link, predecessor, link, &block->parents) {
> +      /* Remove block from all of its predecessors' successor lists. */
> +      foreach_list_typed_safe (bblock_link, successor, link,
> +                               &predecessor->block->children) {
> +         if (block == successor->block) {
> +            successor->link.remove();
> +            ralloc_free(successor);
> +         }
> +      }
> +
> +      /* Add removed-block's successors to its predecessors' successor lists. */

I think this is overflowing 80 columns.

> +      foreach_list_typed (bblock_link, successor, link, &block->children) {
> +         if (!successor->block->is_successor_of(predecessor->block)) {
> +            predecessor->block->children.push_tail(link(mem_ctx,
> +                                                        successor->block));
> +         }
> +      }
> +   }
> +
> +   foreach_list_typed_safe (bblock_link, successor, link, &block->children) {
> +      /* Remove block from all of its childrens' parents lists. */
> +      foreach_list_typed_safe (bblock_link, predecessor, link,
> +                               &successor->block->parents) {
> +         if (block == predecessor->block) {
> +            predecessor->link.remove();
> +            ralloc_free(predecessor);
> +         }
> +      }
> +
> +      /* Add removed-block's predecessors to its successors' predecessor lists. */

Same here.

> +      foreach_list_typed (bblock_link, predecessor, link, &block->parents) {
> +         if (!predecessor->block->is_predecessor_of(successor->block)) {
> +            successor->block->parents.push_tail(link(mem_ctx,
> +                                                     predecessor->block));
> +         }
> +      }
> +   }
> +
> +   block->link.remove();
> +
> +   for (int b = block->num; b < this->num_blocks - 1; b++) {
> +      this->blocks[b] = this->blocks[b + 1];
> +      this->blocks[b]->num = b;
> +   }
> +
> +   this->blocks[this->num_blocks - 1]->num = this->num_blocks - 2;
> +   this->num_blocks--;
> +}
> +
>  bblock_t *
>  cfg_t::new_block()
>  {
> -   bblock_t *block = new(mem_ctx) bblock_t();
> +   bblock_t *block = new(mem_ctx) bblock_t(this);
>  
>     return block;
>  }
> diff --git a/src/mesa/drivers/dri/i965/brw_cfg.h b/src/mesa/drivers/dri/i965/brw_cfg.h
> index a688870..35ee29a 100644
> --- a/src/mesa/drivers/dri/i965/brw_cfg.h
> +++ b/src/mesa/drivers/dri/i965/brw_cfg.h
> @@ -55,7 +55,7 @@ struct bblock_t {
>  #ifdef __cplusplus
>     DECLARE_RALLOC_CXX_OPERATORS(bblock_t)
>  
> -   bblock_t();
> +   bblock_t(cfg_t *cfg);

Just to be safe I would guard this with "explicit".

      explicit bblock_t(cfg_t *cfg);

I don't think there is any need for us to allow compiler to generate a
conversion from cfg_t to bblock_t via constructor. I at least would be more
at rest if we allowed automatic construction only when it is really needed.

With that and with the introduction of the new member (the actual list
manipulation looks sound):

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

>  
>     void add_successor(void *mem_ctx, bblock_t *successor);
>     bool is_predecessor_of(const bblock_t *block) const;
> @@ -93,6 +93,8 @@ struct cfg_t {
>     cfg_t(exec_list *instructions);
>     ~cfg_t();
>  
> +   void remove_block(bblock_t *block);
> +
>     bblock_t *new_block();
>     void set_next_block(bblock_t **cur, bblock_t *block, int ip);
>     void make_block_array();
> -- 
> 1.8.5.5
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev