On Tue, Apr 10, 2018 at 12:52 PM, Caio Marcelo de Oliveira Filho < caio.olive...@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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev