<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Thu, Apr 28, 2016 at 12:10 AM, Ian Romanick <<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 04/27/2016 07:39 PM, Jason Ekstrand wrote: > On Wed, Apr 27, 2016 at 12:54 AM, Ian Romanick <<a href="mailto:idr@freedesktop.org">idr@freedesktop.org</a> <div><div class="h5">> <mailto:<a href="mailto:idr@freedesktop.org">idr@freedesktop.org</a>>> wrote: > > On 04/27/2016 05:41 AM, Jason Ekstrand wrote: > > This matches the "foreach x in container" pattern found in many other > > programming languages. Generated by the following regular expression: > > > > > s/nir_foreach_phi_src(\([^,]*\),\s*\([^,]*\))/nir_foreach_phi_src(\2, \1)/ > > > > and a similar expression for nir_foreach_phi_src_safe. > > --- > > src/compiler/nir/nir.c | 2 +- > > src/compiler/nir/nir.h | 8 > ++++---- > > src/compiler/nir/nir_control_flow.c | 4 ++-- > > src/compiler/nir/nir_from_ssa.c | 4 ++-- > > src/compiler/nir/nir_instr_set.c | 6 +++--- > > src/compiler/nir/nir_liveness.c | 2 +- > > src/compiler/nir/nir_lower_phis_to_scalar.c | 4 ++-- > > src/compiler/nir/nir_opt_dead_cf.c | 2 +- > > src/compiler/nir/nir_opt_gcm.c | 2 +- > > src/compiler/nir/nir_opt_peephole_select.c | 2 +- > > src/compiler/nir/nir_opt_remove_phis.c | 2 +- > > src/compiler/nir/nir_print.c | 2 +- > > src/compiler/nir/nir_to_ssa.c | 2 +- > > src/compiler/nir/nir_validate.c | 2 +- > > src/gallium/drivers/freedreno/ir3/ir3_nir_lower_if_else.c | 2 +- > > 15 files changed, 23 insertions(+), 23 deletions(-) > > > > diff --git a/src/compiler/nir/nir.c b/src/compiler/nir/nir.c > > index 6c1b129..4c283ef 100644 > > --- a/src/compiler/nir/nir.c > > +++ b/src/compiler/nir/nir.c > > @@ -1162,7 +1162,7 @@ visit_load_const_src(nir_load_const_instr > *instr, nir_foreach_src_cb cb, > > static bool > > visit_phi_src(nir_phi_instr *instr, nir_foreach_src_cb cb, void > *state) > > { > > - nir_foreach_phi_src(instr, src) { > > + nir_foreach_phi_src(src, instr) { > > if (!visit_src(&src->src, cb, state)) > > return false; > > } > > diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h > > index 113073f..ee6abdd 100644 > > --- a/src/compiler/nir/nir.h > > +++ b/src/compiler/nir/nir.h > > @@ -1304,10 +1304,10 @@ typedef struct { > > nir_src src; > > } nir_phi_src; > > > > -#define nir_foreach_phi_src(phi, entry) \ > > - foreach_list_typed(nir_phi_src, entry, node, &(phi)->srcs) > > -#define nir_foreach_phi_src_safe(phi, entry) \ > > - foreach_list_typed_safe(nir_phi_src, entry, node, &(phi)->srcs) > > Should the macro be renamed nir_foreach_src_phi? That way the words in > the name match the order of the parameters. That also makes the name > (almost) short for "for each source in phi". Maybe > nir_foreach_src_in_phi? I think Matt previously nixed having "in" in > these macro names, so I don't want to reopen a previously closed can of > worms. > > > That's a good question and I'm glad you asked it. Short version: I > don't know. I'll give it some thought. > > Slightly longer version: The current naming scheme for the foreach > macros is that nir_foreach_foo iterates over things of type nir_foo so > nir_foreach_variable is for nir_variable, nir_foreach_block is for > nir_block, and nir_foreach_phi_src is for nir_phi_src. The fact that > it's called nir_foreach_phi_src and walks over every source in a phi is > kind of an accident. </div></div>Ah. I didn't realize it was iterating a phi_src. I was just looking at the names of the parameters. Maybe changing the parameter names to (phi_src, phi) would make it more obvious to newbs.</blockquote><div> </div><div>Consider it done. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> *shrug* Now that you've explained it, the macro name is fine with me as is. <div class="HOEnZb"><div class="h5"> > On the other hand, you're absolutely write that it does look a lot like > "foreach src in phi" which would also be perfectly sensible. To be > honest, I've never thought about it that way; I've always read it as > "foreach phi_src" so this is a thought that has never occured to me. > > I'll give it a bit more thought. I think I still have a very slight > leaning towards keeping it named the way it is but I'll think about it a > bit more. I could also be easily swayed by someone with a strong > opinion. ;-) > > > > +#define nir_foreach_phi_src(src, phi) \ > > + foreach_list_typed(nir_phi_src, src, node, &(phi)->srcs) > > +#define nir_foreach_phi_src_safe(src, phi) \ > > + foreach_list_typed_safe(nir_phi_src, src, node, &(phi)->srcs) > > > > typedef struct { > > nir_instr instr; > > diff --git a/src/compiler/nir/nir_control_flow.c > b/src/compiler/nir/nir_control_flow.c > > index 64d9a86..a485e71 100644 > > --- a/src/compiler/nir/nir_control_flow.c > > +++ b/src/compiler/nir/nir_control_flow.c > > @@ -261,7 +261,7 @@ rewrite_phi_preds(nir_block *block, nir_block > *old_pred, nir_block *new_pred) > > break; > > > > nir_phi_instr *phi = nir_instr_as_phi(instr); > > - nir_foreach_phi_src(phi, src) { > > + nir_foreach_phi_src(src, phi) { > > if (src->pred == old_pred) { > > src->pred = new_pred; > > break; > > @@ -542,7 +542,7 @@ remove_phi_src(nir_block *block, nir_block *pred) > > break; > > > > nir_phi_instr *phi = nir_instr_as_phi(instr); > > - nir_foreach_phi_src_safe(phi, src) { > > + nir_foreach_phi_src_safe(src, phi) { > > if (src->pred == pred) { > > list_del(&src->src.use_link); > > exec_node_remove(&src->node); > > diff --git a/src/compiler/nir/nir_from_ssa.c > b/src/compiler/nir/nir_from_ssa.c > > index 947ebe1..d333752 100644 > > --- a/src/compiler/nir/nir_from_ssa.c > > +++ b/src/compiler/nir/nir_from_ssa.c > > @@ -331,7 +331,7 @@ isolate_phi_nodes_block(nir_block *block, void > *dead_ctx) > > > > nir_phi_instr *phi = nir_instr_as_phi(instr); > > assert(phi->dest.is_ssa); > > - nir_foreach_phi_src(phi, src) { > > + nir_foreach_phi_src(src, phi) { > > nir_parallel_copy_instr *pcopy = > > get_parallel_copy_at_end_of_block(src->pred); > > assert(pcopy); > > @@ -380,7 +380,7 @@ coalesce_phi_nodes_block(nir_block *block, > struct from_ssa_state *state) > > assert(phi->dest.is_ssa); > > merge_node *dest_node = get_merge_node(&phi->dest.ssa, state); > > > > - nir_foreach_phi_src(phi, src) { > > + nir_foreach_phi_src(src, phi) { > > assert(src->src.is_ssa); > > merge_node *src_node = get_merge_node(src->src.ssa, state); > > if (src_node->set != dest_node->set) > > diff --git a/src/compiler/nir/nir_instr_set.c > b/src/compiler/nir/nir_instr_set.c > > index c616143..fe312a0 100644 > > --- a/src/compiler/nir/nir_instr_set.c > > +++ b/src/compiler/nir/nir_instr_set.c > > @@ -106,7 +106,7 @@ hash_phi(uint32_t hash, const nir_phi_instr > *instr) > > unsigned num_preds = instr->instr.block->predecessors->entries; > > NIR_VLA(nir_phi_src *, srcs, num_preds); > > unsigned i = 0; > > - nir_foreach_phi_src(instr, src) { > > + nir_foreach_phi_src(src, instr) { > > srcs[i++] = src; > > } > > > > @@ -343,8 +343,8 @@ nir_instrs_equal(const nir_instr *instr1, > const nir_instr *instr2) > > if (phi1->instr.block != phi2->instr.block) > > return false; > > > > - nir_foreach_phi_src(phi1, src1) { > > - nir_foreach_phi_src(phi2, src2) { > > + nir_foreach_phi_src(src1, phi1) { > > + nir_foreach_phi_src(src2, phi2) { > > if (src1->pred == src2->pred) { > > if (!nir_srcs_equal(src1->src, src2->src)) > > return false; > > diff --git a/src/compiler/nir/nir_liveness.c > b/src/compiler/nir/nir_liveness.c > > index 69c6fd9..1fcb01d 100644 > > --- a/src/compiler/nir/nir_liveness.c > > +++ b/src/compiler/nir/nir_liveness.c > > @@ -138,7 +138,7 @@ propagate_across_edge(nir_block *pred, > nir_block *succ, > > break; > > nir_phi_instr *phi = nir_instr_as_phi(instr); > > > > - nir_foreach_phi_src(phi, src) { > > + nir_foreach_phi_src(src, phi) { > > if (src->pred == pred) { > > set_src_live(&src->src, live); > > break; > > diff --git a/src/compiler/nir/nir_lower_phis_to_scalar.c > b/src/compiler/nir/nir_lower_phis_to_scalar.c > > index a1a679b..0e27e6d 100644 > > --- a/src/compiler/nir/nir_lower_phis_to_scalar.c > > +++ b/src/compiler/nir/nir_lower_phis_to_scalar.c > > @@ -146,7 +146,7 @@ should_lower_phi(nir_phi_instr *phi, struct > lower_phis_to_scalar_state *state) > > > > bool scalarizable = true; > > > > - nir_foreach_phi_src(phi, src) { > > + nir_foreach_phi_src(src, phi) { > > scalarizable = is_phi_src_scalarizable(src, state); > > if (!scalarizable) > > break; > > @@ -214,7 +214,7 @@ lower_phis_to_scalar_block(nir_block *block, > > > > vec->src[i].src = nir_src_for_ssa(&new_phi->dest.ssa); > > > > - nir_foreach_phi_src(phi, src) { > > + nir_foreach_phi_src(src, phi) { > > /* We need to insert a mov to grab the i'th component > of src */ > > nir_alu_instr *mov = nir_alu_instr_create(state->mem_ctx, > > nir_op_imov); > > diff --git a/src/compiler/nir/nir_opt_dead_cf.c > b/src/compiler/nir/nir_opt_dead_cf.c > > index ca73e50..4b62e55 100644 > > --- a/src/compiler/nir/nir_opt_dead_cf.c > > +++ b/src/compiler/nir/nir_opt_dead_cf.c > > @@ -97,7 +97,7 @@ opt_constant_if(nir_if *if_stmt, bool condition) > > > > nir_phi_instr *phi = nir_instr_as_phi(instr); > > nir_ssa_def *def = NULL; > > - nir_foreach_phi_src(phi, phi_src) { > > + nir_foreach_phi_src(phi_src, phi) { > > if (phi_src->pred != last_block) > > continue; > > > > diff --git a/src/compiler/nir/nir_opt_gcm.c > b/src/compiler/nir/nir_opt_gcm.c > > index d79235d..a87a006 100644 > > --- a/src/compiler/nir/nir_opt_gcm.c > > +++ b/src/compiler/nir/nir_opt_gcm.c > > @@ -292,7 +292,7 @@ gcm_schedule_late_def(nir_ssa_def *def, void > *void_state) > > if (use_instr->type == nir_instr_type_phi) { > > nir_phi_instr *phi = nir_instr_as_phi(use_instr); > > > > - nir_foreach_phi_src(phi, phi_src) { > > + nir_foreach_phi_src(phi_src, phi) { > > if (phi_src->src.ssa == def) > > lca = nir_dominance_lca(lca, phi_src->pred); > > } > > diff --git a/src/compiler/nir/nir_opt_peephole_select.c > b/src/compiler/nir/nir_opt_peephole_select.c > > index b31cc35..4d2b74d 100644 > > --- a/src/compiler/nir/nir_opt_peephole_select.c > > +++ b/src/compiler/nir/nir_opt_peephole_select.c > > @@ -194,7 +194,7 @@ nir_opt_peephole_select_block(nir_block > *block, void *mem_ctx) > > memset(sel->src[0].swizzle, 0, sizeof sel->src[0].swizzle); > > > > assert(exec_list_length(&phi->srcs) == 2); > > - nir_foreach_phi_src(phi, src) { > > + nir_foreach_phi_src(src, phi) { > > assert(src->pred == then_block || src->pred == else_block); > > assert(src->src.is_ssa); > > > > diff --git a/src/compiler/nir/nir_opt_remove_phis.c > b/src/compiler/nir/nir_opt_remove_phis.c > > index 5234c0f..5d015c2 100644 > > --- a/src/compiler/nir/nir_opt_remove_phis.c > > +++ b/src/compiler/nir/nir_opt_remove_phis.c > > @@ -56,7 +56,7 @@ remove_phis_block(nir_block *block) > > nir_ssa_def *def = NULL; > > bool srcs_same = true; > > > > - nir_foreach_phi_src(phi, src) { > > + nir_foreach_phi_src(src, phi) { > > assert(src->src.is_ssa); > > > > /* For phi nodes at the beginning of loops, we may > encounter some > > diff --git a/src/compiler/nir/nir_print.c > b/src/compiler/nir/nir_print.c > > index 76effa9..e504d88 100644 > > --- a/src/compiler/nir/nir_print.c > > +++ b/src/compiler/nir/nir_print.c > > @@ -793,7 +793,7 @@ print_phi_instr(nir_phi_instr *instr, > print_state *state) > > FILE *fp = state->fp; > > print_dest(&instr->dest, state); > > fprintf(fp, " = phi "); > > - nir_foreach_phi_src(instr, src) { > > + nir_foreach_phi_src(src, instr) { > > if (&src->node != exec_list_get_head(&instr->srcs)) > > fprintf(fp, ", "); > > > > diff --git a/src/compiler/nir/nir_to_ssa.c > b/src/compiler/nir/nir_to_ssa.c > > index 7668491..7dc283d 100644 > > --- a/src/compiler/nir/nir_to_ssa.c > > +++ b/src/compiler/nir/nir_to_ssa.c > > @@ -389,7 +389,7 @@ rewrite_phi_sources(nir_block *block, > nir_block *pred, rewrite_state *state) > > > > state->parent_instr = instr; > > > > - nir_foreach_phi_src(phi_instr, src) { > > + nir_foreach_phi_src(src, phi_instr) { > > if (src->pred == pred) { > > rewrite_use(&src->src, state); > > break; > > diff --git a/src/compiler/nir/nir_validate.c > b/src/compiler/nir/nir_validate.c > > index 10a7832..e928c41 100644 > > --- a/src/compiler/nir/nir_validate.c > > +++ b/src/compiler/nir/nir_validate.c > > @@ -588,7 +588,7 @@ validate_phi_src(nir_phi_instr *instr, > nir_block *pred, validate_state *state) > > assert(instr->dest.is_ssa); > > > > exec_list_validate(&instr->srcs); > > - nir_foreach_phi_src(instr, src) { > > + nir_foreach_phi_src(src, instr) { > > if (src->pred == pred) { > > assert(src->src.is_ssa); > > assert(src->src.ssa->num_components == > > diff --git > a/src/gallium/drivers/freedreno/ir3/ir3_nir_lower_if_else.c > b/src/gallium/drivers/freedreno/ir3/ir3_nir_lower_if_else.c > > index 09c93da..8bd70e2 100644 > > --- a/src/gallium/drivers/freedreno/ir3/ir3_nir_lower_if_else.c > > +++ b/src/gallium/drivers/freedreno/ir3/ir3_nir_lower_if_else.c > > @@ -273,7 +273,7 @@ lower_if_else_block(nir_block *block, > nir_builder *b, void *mem_ctx) > > memset(sel->src[0].swizzle, 0, sizeof > sel->src[0].swizzle); > > > > assert(exec_list_length(&phi->srcs) == 2); > > - nir_foreach_phi_src(phi, src) { > > + nir_foreach_phi_src(src, phi) { > > assert(src->pred == then_block || src->pred > == else_block); > > assert(src->src.is_ssa); > > > > > > </div></div></blockquote></div> </div></div>