[Mesa-dev] [PATCH] glsl: track total amount of uniform locations used

[Timothy Arceri]

On Tue, 2016-01-12 at 07:59 +0200, Tapani Pälli wrote:
> 
> On 01/12/2016 12:30 AM, Timothy Arceri wrote:
> > On Mon, 2016-01-11 at 08:24 +0200, Tapani Pälli wrote:
> > > 
> > > On 01/08/2016 11:32 AM, Tapani Pälli wrote:
> > > > 
> > > > 
> > > > On 01/08/2016 11:17 AM, Timothy Arceri wrote:
> > > > > On Fri, 2016-01-08 at 08:20 +0200, Tapani Pälli wrote:
> > > > > > Linker missed a check for situation where we exceed max
> > > > > > amount
> > > > > > of
> > > > > > uniform locations with explicit + implicit locations. Patch
> > > > > > adds this
> > > > > > check to already existing iteration over uniforms in
> > > > > > linker.
> > > > > > 
> > > > > > Fixes following CTS test:
> > > > > >      ES31-CTS.explicit_uniform_location.uniform-loc
> > > > > > -negative
> > > > > > -link-max
> > > > > > -num-of-locations
> > > > > > 
> > > > > > Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
> > > > > > ---
> > > > > >    src/glsl/linker.cpp | 17 +++++++++++++++--
> > > > > >    1 file changed, 15 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> > > > > > index 7a18523..ef23b36 100644
> > > > > > --- a/src/glsl/linker.cpp
> > > > > > +++ b/src/glsl/linker.cpp
> > > > > > @@ -3130,6 +3130,7 @@
> > > > > > check_explicit_uniform_locations(struct
> > > > > > gl_context *ctx,
> > > > > >          return;
> > > > > >       }
> > > > > > 
> > > > > > +   unsigned entries_total = 0;
> > > > > >       for (unsigned i = 0; i < MESA_SHADER_STAGES; i++) {
> > > > > >          struct gl_shader *sh = prog->_LinkedShaders[i];
> > > > > > 
> > > > > > @@ -3138,8 +3139,12 @@
> > > > > > check_explicit_uniform_locations(struct
> > > > > > gl_context *ctx,
> > > > > > 
> > > > > >          foreach_in_list(ir_instruction, node, sh->ir) {
> > > > > >             ir_variable *var = node->as_variable();
> > > > > > -         if (var && (var->data.mode == ir_var_uniform &&
> > > > > > -                     var->data.explicit_location)) {
> > > > > > +         if (!var || var->data.mode != ir_var_uniform)
> > > > > > +            continue;
> > > > > > +
> > > > > > +         entries_total += var->type->is_array() ? var
> > > > > > ->type
> > > > > > ->length
> > > > > > : 1;
> > 
> > This should be:
> > 
> > entries_total += var->type->uniform_locations();
> > 
> > Otherwise AoA and structs will not be counted correctly.
> 
> ah I had forgotten about uniform_locations(), thanks!
> 
> > 
> > > > > > +
> > > > > > +         if (var->data.explicit_location) {
> > > > > >                bool ret;
> > > > > >                if (var->type->is_subroutine())
> > > > > >                   ret =
> > > > > > reserve_subroutine_explicit_locations(prog, sh,
> > > > > > var);
> > > > > > @@ -3153,6 +3158,14 @@
> > > > > > check_explicit_uniform_locations(struct
> > > > > > gl_context *ctx,
> > > > > >          }
> > > > > >       }
> > > > > > 
> > > > > > +   /* Verify that total amount of entries for explicit and
> > > > > > implicit
> > > > > > locations
> > > > > > +    * is less than MAX_UNIFORM_LOCATIONS.
> > > > > > +    */
> > > > > > +   if (entries_total >= ctx
> > > > > > ->Const.MaxUserAssignableUniformLocations) {
> > > > > > +      linker_error(prog, "count of uniform locations >=
> > > > > > MAX_UNIFORM_LOCATIONS"
> > > > > > +                   "(%u >= %u)", entries_total,
> > > > > > +                   ctx
> > > > > > ->Const.MaxUserAssignableUniformLocations);
> > > > > > +   }
> > > > > 
> > > > > I think this check would be better in
> > > > > link_assign_uniform_locations()
> > > > > because check_explicit_uniform_locations() is called before
> > > > > arrays are
> > > > > resized via update_array_sizes()
> > > > > 
> > > > > Also in link_assign_uniform_locations() there is already a
> > > > > count
> > > > > of
> > > > > uniform locations.
> > > > > 
> > > > > See: const unsigned num_uniforms =
> > > > > uniform_size.num_active_uniforms;
> > > > 
> > > > That function can't currently fail and it does not know the max
> > > > no
> > > > of
> > > > uniforms so it was easier to plug here. I can move it there but
> > > > it
> > > > requires changing the function signature a bit, I guess not
> > > > that
> > > > big
> > > > deal though.
> > > 
> > > I've made a version that counts used locations only at
> > > link_assign_uniform_locations(). Problem with that is that it
> > > happens
> > > too late. With the test case an uniform array gets optimized away
> > > (since
> > > it's not used in the shader, however as it is using explicit
> > > location,
> > > those locations must be reserved), therefore this happens too
> > > late.
> > > IMO
> > > we must do this calculation as the my first proposal did it.
> > 
> > Hmm, sure they must be reserved but if they are inactive I don't
> > think
> > there is anything in the spec that disallows using the location for
> > varyings that don't have an explicit location.
> 
> I don't think overlap between these matter as these are distinct 
> resources, used for different things.

Blah, right I seem to have forgotten this patch was about uniforms for
a second there.

> 
> > Anyways its not a big deal I guess, if you fix up the issue above
> > you
> > can have.
> > 
> > Reviewed-by: Timothy Arceri <timothy.arceri at collabora.com>
> 
> Thanks for the review!
> 
> // Tapani
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev