<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Apr 10, 2018 at 12:52 PM, Caio Marcelo de Oliveira Filho <<a href="mailto:caio.oliveira@intel.com" target="_blank">caio.oliveira@intel.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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. </blockquote><div> </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> --- 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); </blockquote><div> </div><div>When a block contains multiple lines, we use { } regardless of whether or not it's just a single statement. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + 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); </blockquote><div> </div><div>Same here. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + 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; </blockquote><div> </div><div>I think I still like the early return a tad bit better. I'm not going to be insistent though. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> - - 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 <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div> </div></div>