On Feb 2, 2015 10:59 PM, "Kenneth Graunke" <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> wrote: > > On Monday, February 02, 2015 10:28:22 PM Jason Ekstrand wrote: > > On Mon, Feb 2, 2015 at 9:08 PM, Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> > > 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 <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> Thanks! > with the concern that non-SSA phi sources still sound bizarre.