[Mesa-dev] [PATCH v5 1/3] nir: Add a pass to lower vector phi nodes to scalar phi nodes

[Jason Ekstrand]

On Feb 2, 2015 10:59 PM, "Kenneth Graunke" <kenneth at whitecape.org> wrote:
>
> On Monday, February 02, 2015 10:28:22 PM Jason Ekstrand wrote:
> > On Mon, Feb 2, 2015 at 9:08 PM, Kenneth Graunke <kenneth at whitecape.org>
> > wrote:
> >
> > > On Thursday, January 29, 2015 01:40:18 PM Jason Ekstrand wrote:
> [snip]
> > > > +static bool
> > > > +is_phi_src_scalarizable(nir_phi_src *src,
> > > > +                        struct lower_phis_to_scalar_state *state)
> > > > +{
> > > > +   /* Don't know what to do with non-ssa sources */
> > > > +   if (!src->src.is_ssa)
> > >
> > > Can phi nodes with non-SSA sources even exist?  That sounds crazy.
> > > Perhaps just assert(src->src.is_ssa)?
> > >
> >
> > It's possible.  We should probably disallow it as we can't coalesce them
> > anyway, but it can happen at the moment.
>
> Really?  That sounds scary - what does phi(non-SSA values) even mean?

Yeah, I'll look into that and get it disallowed. We shouldn't be getting
non-SSA phi sources.  I just seem to remember them happening for some
reason.

> [snip]
> > > Perhaps add:
> > >
> > >       case nir_intrinsic_load_uniform:
> > >       case nir_intrinsic_load_const:
> > >          return true;
> > >
> > > so that the pass could handle this same concept after nir_lower_io()?
> > >
> >
> > I have a patch floating arround that adds load_const and it helps by a
few
> > instructions.  We should probably do the same for inputs and uniforms
but I
> > thought I had some reason not to.  Can't remember what it is now.
>
> Huh.  I'm not sure why it'd break - you're already doing it for input
> and uniform /variables/...

Right... Sure, I'll add that.

> [snip]
> > >
> > > > +    * won't automatically make us fail to scalarize.
> > > > +    */
> > > > +   entry = _mesa_hash_table_insert(state->phi_table, phi, (void
> > > *)(intptr_t)1);
> > >
> > > This is weird.  Why are you using a hash table and not a set?  It
sounds
> > > like you're trying to store a tri-state: unknown (no entry in hash
> > > table), known-not-scalarizable (entry in hash table, value of 0), and
> > > known-scalarizable (entry in hash table, value of 1).
> > >
> > > However, I don't see any code that actually inserts or updates a value
> > > to 0 - are you missing some code?
> > >
> > > Otherwise, this looks good to me...
> > >
> > > > +
> > > > +   bool scalarizable = true;
> > > > +
> > > > +   nir_foreach_phi_src(phi, src) {
> > > > +      scalarizable = is_phi_src_scalarizable(src, state);
> > > > +      if (!scalarizable)
> > > > +         break;
> > > > +   }
> > > > +
> > > > +   entry->data = (void *)(intptr_t)scalarizable;
> > >
> >
> > It's updated right here ^^
>
> So it is!  I was expecting to see a search/remove/insert, and overlooked
> the fact that you're getting a hash_entry pointer (rather than the
> value) and editing the data.  That seems fine.
>
> This is:
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

Thanks!

> with the concern that non-SSA phi sources still sound bizarre.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150203/811dd664/attachment.html>