[Mesa-dev] [PATCH 06/78] i965/nir/vec4: Add setup of uniform variables

[Kenneth Graunke]

On Tuesday, June 30, 2015 10:04:47 AM Iago Toral wrote:
> Hi Jason,
> 
> On Mon, 2015-06-29 at 16:22 -0700, Jason Ekstrand wrote:
> > On Fri, Jun 26, 2015 at 1:06 AM, Eduardo Lima Mitev <elima at igalia.com> wrote:
> > > From: Iago Toral Quiroga <itoral at igalia.com>
> > >
> > > This is based on similar code existing in vec4_visitor. It builds the
> > > uniform register file iterating through each uniform variable. It
> > > also stores the index of each register at the corresponding offset
> > > in a map. This map will later be used by load_uniform intrinsic
> > > instructions to build the correct UNIFORM source register.
> > >
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89580
> > > ---
> > >  src/mesa/drivers/dri/i965/brw_vec4.h       |   2 +
> > >  src/mesa/drivers/dri/i965/brw_vec4_nir.cpp | 115 ++++++++++++++++++++++++++++-
> > >  2 files changed, 114 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/mesa/drivers/dri/i965/brw_vec4.h b/src/mesa/drivers/dri/i965/brw_vec4.h
> > > index 673df4e..6535f19 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_vec4.h
> > > +++ b/src/mesa/drivers/dri/i965/brw_vec4.h
> > > @@ -414,6 +414,8 @@ public:
> > >     src_reg *nir_inputs;
> > >     int *nir_outputs;
> > >     brw_reg_type *nir_output_types;
> > > +   unsigned *nir_uniform_offset;
> > > +   unsigned *nir_uniform_driver_location;
> > >
> > >  protected:
> > >     void emit_vertex();
> > > diff --git a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
> > > index 2d457a6..40ec66f 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
> > > +++ b/src/mesa/drivers/dri/i965/brw_vec4_nir.cpp
> > > @@ -106,19 +106,128 @@ vec4_visitor::nir_setup_outputs(nir_shader *shader)
> > >  void
> > >  vec4_visitor::nir_setup_uniforms(nir_shader *shader)
> > >  {
> > > -   /* @TODO: Not yet implemented */
> > > +   uniforms = 0;
> > > +
> > > +   nir_uniform_offset =
> > > +      rzalloc_array(mem_ctx, unsigned, this->uniform_array_size);
> > > +   memset(nir_uniform_offset, 0, this->uniform_array_size * sizeof(unsigned));
> > 
> > rzalloc memsets the whole thing to 0 for you, this memset is redundant.
> > 
> > > +
> > > +   nir_uniform_driver_location =
> > > +      rzalloc_array(mem_ctx, unsigned, this->uniform_array_size);
> > > +   memset(nir_uniform_driver_location, 0,
> > > +          this->uniform_array_size * sizeof(unsigned));
> > 
> > Same here.
> 
> Oh, right.
> 
> > > +
> > > +   if (shader_prog) {
> > > +      foreach_list_typed(nir_variable, var, node, &shader->uniforms) {
> > > +         /* UBO's, atomics and samplers don't take up space in the
> > > +            uniform file */
> > > +         if (var->interface_type != NULL || var->type->contains_atomic() ||
> > > +             type_size(var->type) == 0) {
> > 
> > I'm curious as to why you have this extra type_size() == 0 condition.
> > We don't have that in the FS NIR code.  What caused you to add it?
> 
> Take this piglit test for example:
> bin/textureSize vs isampler1D -auto -fbo
> 
> here, 'tex' is a uniform of size 0 since type_size() returns 0 for all
> sampler types. If we do not ignore these, we will try to store uniform
> information for them in the various structures we have to track uniform
> data, like uniform_size[] and others. The size allocated for these
> arrays is computed by in the vec4_visitor constructor based on
> stage_prog_data->nr_params (uniform_array_size) and that does not seem
> to make room for zero-sized uniforms. Without that check we would
> process more uniforms than uniform_array_size and overflow the arrays we
> allocate to track uniform information. I understand that
> stage_prog_data->nr_params does not track uniforms that don't use
> register space, so skipping uniforms with no size seems to make sense
> here.
> 
> Notice that this is done in the current vec4_visitor too, when we visit
> the variable in vec4_visitor::visit(ir_variable *ir), for ir_var_uniform
> there is this code:
> 
> if (ir->is_in_uniform_block() || type_size(ir->type) == 0)
>     return; 

Oh, right.  I think we handle sampler uniforms a bit differently in the
vec4 and FS backends, and I was never quite sure why...I know we've had
bugs relating to zero-sized uniforms, but I was never quite able to sort
them out.

This seems fine for now - it keeps the vec4 world doing what it's always
done.


-------------- 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/20150709/0243168c/attachment-0001.sig>