On Monday, February 02, 2015 10:28:22 PM Jason Ekstrand wrote:
> On Mon, Feb 2, 2015 at 9:08 PM, Kenneth Graunke <kenn...@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 <kenn...@whitecape.org>
with the concern that non-SSA phi sources still sound bizarre.
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
