[Mesa-dev] [PATCH 2/6] nir: Switch the arguments to nir_foreach_phi_src
Connor Abbott
cwabbott0 at gmail.com
Wed Apr 27 18:52:45 UTC 2016
On Wed, Apr 27, 2016 at 1:39 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
> On Wed, Apr 27, 2016 at 12:54 AM, Ian Romanick <idr at freedesktop.org> 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.
>
> 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. ;-)
My opinion is we should keep it, for the reason you mentioned: it's
more consistent with the naming scheme of the rest of the iterators.
Even if it seems a little weird in isolation, once you've been working
with all the other iterators for a while, it's a lot more natural to
read it as "for each phi source" than "for each source in (the) phi."
Also, the former way of reading it gives more of a clue as to why this
macro is a separate thing from nir_foreach_src: it iterates over phi
sources instead of normal sources. Unlike normal sources, phi sources
also have their associated basic block attached.
>
>>
>> > +#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);
>> >
>> >
>>
>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
More information about the mesa-dev
mailing list