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

[Timothy Arceri]

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.


> > > > +
> > > > +         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.

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>


> 
> 
> > 
> > > 
> > > 
> > > >      delete uniform_map;
> > > >   }
> > > > 
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev