[Mesa-dev] [PATCH 1/2] glsl: fix slot_end calculations and simplify reserved_slots check

[Timothy Arceri]

On Sat, 2016-11-05 at 14:56 -0400, Ilia Mirkin wrote:
> On Sat, Nov 5, 2016 at 1:03 PM, Ilia Mirkin <imirkin at alum.mit.edu>
> wrote:
> > 
> > The previous code was confused about whether slot_end was inclusive
> > or
> > exclusive. Make it so that it is inclusive consistently, and use it
> > for
> > setting the new location. This should also better deal with arrays
> > of
> > incomplete size, since the slot check will assume it gets packed.
> > 
> > Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
> > ---
> > 
> > No regressions in Intel's jenkins runs, which I believe hit both
> > piglit and CTS.
> > This is somewhat subtle code, but I think I covered what it was
> > trying to do.
> > 
> >  src/compiler/glsl/link_varyings.cpp | 50 ++++++++++++++++---------
> > ------------
> >  1 file changed, 22 insertions(+), 28 deletions(-)
> > 
> > diff --git a/src/compiler/glsl/link_varyings.cpp
> > b/src/compiler/glsl/link_varyings.cpp
> > index e622b3e..49843cc 100644
> > --- a/src/compiler/glsl/link_varyings.cpp
> > +++ b/src/compiler/glsl/link_varyings.cpp
> > @@ -1546,14 +1546,16 @@ varying_matches::assign_locations(struct
> > gl_shader_program *prog,
> > 
> >        previous_var_xfb_only = var->data.is_xfb_only;
> > 
> > -      unsigned num_elements =  type-
> > >count_attribute_slots(is_vertex_input);
> > -      unsigned slot_end;
> > -      if (this->disable_varying_packing &&
> > -          !is_varying_packing_safe(type, var))
> > -         slot_end = 4;
> > -      else
> > -         slot_end = type->without_array()->vector_elements;
> > -      slot_end += *location - 1;
> > +      /* The number of components taken up by this variable. For
> > vertex shader
> > +       * inputs, we use the number of slots * 4, as they have
> > different
> > +       * counting rules.
> > +       */
> > +      unsigned num_components = is_vertex_input ?
> > +         type->count_attribute_slots(is_vertex_input) * 4 :
> > +         this->matches[i].num_components;
> > +
> > +      /* The last slot for this variable, inclusive. */
> > +      unsigned slot_end = *location + num_components - 1;
> > 
> >        /* FIXME: We could be smarter in the below code and loop
> > back over
> >         * trying to fill any locations that we skipped because we
> > couldn't pack
> > @@ -1561,29 +1563,21 @@ varying_matches::assign_locations(struct
> > gl_shader_program *prog,
> >         * hit the linking error if we run out of room and suggest
> > they use
> >         * explicit locations.
> >         */
> > -      for (unsigned j = 0; j < num_elements; j++) {
> > -         while ((slot_end < MAX_VARYING * 4u) &&
> > -                ((reserved_slots & (UINT64_C(1) << *location / 4u)
> > ||
> > -                 (reserved_slots & (UINT64_C(1) << slot_end /
> > 4u))))) {
> > -
> > -            *location = ALIGN(*location + 1, 4);
> > -            slot_end = *location;
> > -
> > -            /* reset the counter and try again */
> > -            j = 0;
> > +      while (slot_end < MAX_VARYING * 4u) {
> > +         const unsigned slots = (slot_end / 4u) - (*location / 4u)
> > + 1;
> > +         const uint64_t slot_mask = ((1ull << slots) - 1) <<
> > (*location / 4u);
> > +
> > +         assert(slots > 0);
> > +         if (reserved_slots & slot_mask) {
> > +            *location = ALIGN(slot_end + 1, 4);
> 
> I was a bit overzealous about changing this -- should be
> 

Right. It's much improved with these changes already :)> *location = ALIGN(*location + 1, 4);
> 
> Since the reserved slot could be the first one. I've made this change
> locally. Intel's CI is still happy.
> 
> Various bit arithmetic could be done to improve this, but ... meh.
> Hardly seems worth it.

Right. It's much improved with these changes already :)

I'm not really sure what you mean about arrays of incomplete size but
the logic in this change looks correct to me.

Reviewed-by: Timothy Arceri <timothy.arceri at collabora.com>

> 
> > 
> > +            slot_end = *location + num_components - 1;
> > +            continue;
> >           }
> > 
> > -         /* Increase the slot to make sure there is enough room
> > for next
> > -          * array element.
> > -          */
> > -         if (this->disable_varying_packing &&
> > -             !is_varying_packing_safe(type, var))
> > -            slot_end += 4;
> > -         else
> > -            slot_end += type->without_array()->vector_elements;
> > +         break;
> >        }
> > 
> > -      if (!var->data.patch && *location >= MAX_VARYING * 4u) {
> > +      if (!var->data.patch && slot_end >= MAX_VARYING * 4u) {
> >           linker_error(prog, "insufficient contiguous locations
> > available for "
> >                        "%s it is possible an array or struct could
> > not be "
> >                        "packed between varyings with explicit
> > locations. Try "
> > @@ -1593,7 +1587,7 @@ varying_matches::assign_locations(struct
> > gl_shader_program *prog,
> > 
> >        this->matches[i].generic_location = *location;
> > 
> > -      *location += this->matches[i].num_components;
> > +      *location = slot_end + 1;
> >     }
> > 
> >     return (generic_location + 3) / 4;
> > --
> > 2.7.3
> > 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev