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

[Ilia Mirkin]

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

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

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