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.pa...@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.arc...@collabora.com>
>
> Thanks for the review!
>
> // Tapani
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
