[Mesa-dev] [RFCv2 03/13] nir: allow pre-resolved sampler uniform locations

[Rob Clark]

On Mon, Nov 9, 2015 at 4:08 PM, Timothy Arceri <t_arceri at yahoo.com.au> wrote:
> On Mon, 2015-11-09 at 07:43 -0500, Rob Clark wrote:
>> On Sun, Nov 8, 2015 at 7:58 PM, Timothy Arceri <t_arceri at yahoo.com.au>
>> wrote:
>> > On Sun, 2015-11-08 at 15:12 -0500, Rob Clark wrote:
>> > > From: Rob Clark <robclark at freedesktop.org>
>> > >
>> > > With TGSI, the ir_variable::data.location gets fixed up to be a stage
>> > > local location (rather than program global).  In this case we need to
>> > > skip the UniformStorage[location] lookup.
>> > > ---
>> > >  src/glsl/nir/nir_lower_samplers.c | 23 ++++++++++++++++-------
>> > >  1 file changed, 16 insertions(+), 7 deletions(-)
>> > >
>> > > diff --git a/src/glsl/nir/nir_lower_samplers.c
>> > > b/src/glsl/nir/nir_lower_samplers.c
>> > > index 5df79a6..d99ba4c 100644
>> > > --- a/src/glsl/nir/nir_lower_samplers.c
>> > > +++ b/src/glsl/nir/nir_lower_samplers.c
>> > > @@ -130,14 +130,18 @@ lower_sampler(nir_tex_instr *instr, const struct
>> > > gl_shader_program *shader_progr
>> > >        instr->sampler_array_size = array_elements;
>> > >     }
>> > >
>> > > -   if (location > shader_program->NumUniformStorage - 1 ||
>> > > -       !shader_program->UniformStorage[location].opaque[stage].active)
>> > > {
>> > > -      assert(!"cannot return a sampler");
>> > > -      return;
>> > > -   }
>> > > +   if (!shader_program) {
>> > > +      instr->sampler_index = location;
>> > > +   } else {
>> > > +      if (location > shader_program->NumUniformStorage - 1 ||
>> > > +          !shader_program
>> > > ->UniformStorage[location].opaque[stage].active) {
>> > > +         assert(!"cannot return a sampler");
>> > > +         return;
>> > > +      }
>> > >
>> > > -   instr->sampler_index +=
>> > > -      shader_program->UniformStorage[location].opaque[stage].index;
>> > > +      instr->sampler_index =
>> > > +         shader_program->UniformStorage[location].opaque[stage].index;
>> >
>> > Hi Rob,
>> >
>> > This will break arrays as instr->sampler_index is increamented inside
>> >  calc_sampler_offsets()
>>
>> oh, whoops, I didn't notice that.. ok, that part is easy enough to fix..
>>
>> > calc_sampler_offsets() also modifies the value of location is this what
>> > you
>> > want? I would assume not as we are counting uniforms not just samplers
>> > here.
>>
>> hmm, tbh I'm not entirely sure..  offhand, what piglit's should I
>> check?
>
> tests/spec/arb_gpu_shader5/execution/sampler_array_indexing
>
> Contains the tests you probably want to try out.

oh, hmm, looks like they all need at least gl3.2..

BR,
-R

>>  I guess it would be easier to debug if it worked correctly
>> with glsl_to_tgsi, but I guess I could try to get the non-indirect
>> case working..
>>
>> BR,
>> -R
>>
>> > The other thing to note is that glsl to tgsi doesn't handle indirects on
>> > structs or arrays of arrays correctly (Ilia was trying to fix this).
>> >
>> > Tim
>> >
>> >
>> >
>> > > +   }
>> > >
>> > >     instr->sampler = NULL;
>> > >  }
>> > > @@ -177,6 +181,11 @@ lower_impl(nir_function_impl *impl, const struct
>> > > gl_shader_program *shader_progr
>> > >     nir_foreach_block(impl, lower_block_cb, &state);
>> > >  }
>> > >
>> > > +/* Call with a null 'shader_program' if uniform locations are
>> >
>> > uniform locations -> sampler indices?
>> >
>> > > + * already local to the shader, ie. skipping the
>> > > + * shader_program->UniformStorage[location].opaque[stage].index
>> > > + * lookup
>> > > + */
>> > >  void
>> > >  nir_lower_samplers(nir_shader *shader,
>> > >                     const struct gl_shader_program *shader_program)