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

[Kenneth Graunke]

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?

[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/...

[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>

with the concern that non-SSA phi sources still sound bizarre.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150202/d9aed88b/attachment.sig>