[Mesa-dev] [PATCH 3/3] nir: simplify node matching code when lowering to SSA

[Jason Ekstrand]

On Tue, Apr 10, 2018 at 12:52 PM, Caio Marcelo de Oliveira Filho <
caio.oliveira at intel.com> wrote:

> The matching code doesn't make real use of the return value. The main
> function return value is ignored, and while the worker function
> propagate its return value, the actual callback never returns false.
>

When I wrote this, I don't think I realized it was only going to have the
one use.  I was mostly following the pattern we'd used for all the other
foreach functions in NIR.  That said, I think this is better.


> ---
>  src/compiler/nir/nir_lower_vars_to_ssa.c | 73 +++++++++++-------------
>  1 file changed, 32 insertions(+), 41 deletions(-)
>
> diff --git a/src/compiler/nir/nir_lower_vars_to_ssa.c
> b/src/compiler/nir/nir_lower_vars_to_ssa.c
> index 4c41dd69cf..f84ad067ee 100644
> --- a/src/compiler/nir/nir_lower_vars_to_ssa.c
> +++ b/src/compiler/nir/nir_lower_vars_to_ssa.c
> @@ -217,45 +217,40 @@ get_deref_node(nir_deref_var *deref, struct
> lower_variables_state *state)
>  }
>
>  /* \sa foreach_deref_node_match */
> -static bool
> +static void
>  foreach_deref_node_worker(struct deref_node *node, nir_deref *deref,
> -                          bool (* cb)(struct deref_node *node,
> +                          void (* cb)(struct deref_node *node,
>                                        struct lower_variables_state
> *state),
>                            struct lower_variables_state *state)
>  {
>     if (deref->child == NULL) {
> -      return cb(node, state);
> -   } else {
> -      switch (deref->child->deref_type) {
> -      case nir_deref_type_array: {
> -         nir_deref_array *arr = nir_deref_as_array(deref->child);
> -         assert(arr->deref_array_type == nir_deref_array_type_direct);
> -         if (node->children[arr->base_offset] &&
> -             !foreach_deref_node_worker(node->children[arr->base_offset],
> -                                        deref->child, cb, state))
> -            return false;
> -
> -         if (node->wildcard &&
> -             !foreach_deref_node_worker(node->wildcard,
> -                                        deref->child, cb, state))
> -            return false;
> -
> -         return true;
> -      }
> +      cb(node, state);
> +      return;
> +   }
>
> -      case nir_deref_type_struct: {
> -         nir_deref_struct *str = nir_deref_as_struct(deref->child);
> -         if (node->children[str->index] &&
> -             !foreach_deref_node_worker(node->children[str->index],
> -                                        deref->child, cb, state))
> -            return false;
> +   switch (deref->child->deref_type) {
> +   case nir_deref_type_array: {
> +      nir_deref_array *arr = nir_deref_as_array(deref->child);
> +      assert(arr->deref_array_type == nir_deref_array_type_direct);
> +
> +      if (node->children[arr->base_offset])
> +         foreach_deref_node_worker(node->children[arr->base_offset],
> +                                   deref->child, cb, state);
>

When a block contains multiple lines, we use { } regardless of whether or
not it's just a single statement.


> +      if (node->wildcard)
> +         foreach_deref_node_worker(node->wildcard, deref->child, cb,
> state);
> +      break;
> +   }
>
> -         return true;
> -      }
> +   case nir_deref_type_struct: {
> +      nir_deref_struct *str = nir_deref_as_struct(deref->child);
> +      if (node->children[str->index])
> +         foreach_deref_node_worker(node->children[str->index],
> +                                   deref->child, cb, state);
>

Same here.


> +      break;
> +   }
>
> -      default:
> -         unreachable("Invalid deref child type");
> -      }
> +   default:
> +      unreachable("Invalid deref child type");
>     }
>  }
>
> @@ -271,9 +266,9 @@ foreach_deref_node_worker(struct deref_node *node,
> nir_deref *deref,
>   * The given deref must be a full-length and fully qualified (no wildcards
>   * or indirects) deref chain.
>   */
> -static bool
> +static void
>  foreach_deref_node_match(nir_deref_var *deref,
> -                         bool (* cb)(struct deref_node *node,
> +                         void (* cb)(struct deref_node *node,
>                                       struct lower_variables_state *state),
>                           struct lower_variables_state *state)
>  {
> @@ -281,10 +276,8 @@ foreach_deref_node_match(nir_deref_var *deref,
>     var_deref.deref.child = NULL;
>     struct deref_node *node = get_deref_node(&var_deref, state);
>
> -   if (node == NULL)
> -      return false;
>

I think I still like the early return a tad bit better.  I'm not going to
be insistent though.


> -
> -   return foreach_deref_node_worker(node, &deref->deref, cb, state);
> +   if (node)
> +      foreach_deref_node_worker(node, &deref->deref, cb, state);
>  }
>
>  /* \sa deref_may_be_aliased */
> @@ -440,12 +433,12 @@ register_variable_uses_block(nir_block *block,
>  /* Walks over all of the copy instructions to or from the given deref_node
>   * and lowers them to load/store intrinsics.
>   */
> -static bool
> +static void
>  lower_copies_to_load_store(struct deref_node *node,
>                             struct lower_variables_state *state)
>  {
>     if (!node->copies)
> -      return true;
> +      return;
>
>     struct set_entry *copy_entry;
>     set_foreach(node->copies, copy_entry) {
> @@ -470,8 +463,6 @@ lower_copies_to_load_store(struct deref_node *node,
>     }
>
>     node->copies = NULL;
> -
> -   return true;
>  }
>
>  /* Performs variable renaming
> --
> 2.17.0
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180410/5b7c6546/attachment.html>