[Mesa-dev] [PATCH 06/28] glsl: fix overlapping of varying locations for arrays and structs

Anuj Phogat anuj.phogat at gmail.com
Wed Jan 6 11:46:11 PST 2016


On Mon, Dec 28, 2015 at 9:00 PM, Timothy Arceri
<timothy.arceri at collabora.com> wrote:
> Previously we were only reserving a single location for arrays and
> structs.
>
> We also didn't take into account implicit locations clashing with
> explicit locations when assigning locations for their arrays or
> structs.
>
> This patch fixes both issues.
>
> V5: fix regression for patch inputs/outputs in tessellation shaders
> V4: just use count_attribute_slots() to get the number of slots,
> also calculate the correct number of slots to reserve for gs and
> tess stages by making use of the new get_varying_type() helper.
> V3: handle arrays of structs
> V2: also fix for arrays of arrays and structs.
> ---
>  src/glsl/link_varyings.cpp | 80 +++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 68 insertions(+), 12 deletions(-)
>
> diff --git a/src/glsl/link_varyings.cpp b/src/glsl/link_varyings.cpp
> index d9550df..34e8418 100644
> --- a/src/glsl/link_varyings.cpp
> +++ b/src/glsl/link_varyings.cpp
> @@ -825,7 +825,8 @@ public:
>                     gl_shader_stage consumer_stage);
>     ~varying_matches();
>     void record(ir_variable *producer_var, ir_variable *consumer_var);
> -   unsigned assign_locations(uint64_t reserved_slots, bool separate_shader);
> +   unsigned assign_locations(struct gl_shader_program *prog,
> +                             uint64_t reserved_slots, bool separate_shader);
>     void store_locations() const;
>
>  private:
> @@ -1042,7 +1043,9 @@ varying_matches::record(ir_variable *producer_var, ir_variable *consumer_var)
>   * passed to varying_matches::record().
>   */
>  unsigned
> -varying_matches::assign_locations(uint64_t reserved_slots, bool separate_shader)
> +varying_matches::assign_locations(struct gl_shader_program *prog,
> +                                  uint64_t reserved_slots,
> +                                  bool separate_shader)
>  {
>     /* We disable varying sorting for separate shader programs for the
>      * following reasons:
> @@ -1079,10 +1082,21 @@ varying_matches::assign_locations(uint64_t reserved_slots, bool separate_shader)
>     for (unsigned i = 0; i < this->num_matches; i++) {
>        unsigned *location = &generic_location;
>
> -      if ((this->matches[i].consumer_var &&
> -           this->matches[i].consumer_var->data.patch) ||
> -          (this->matches[i].producer_var &&
> -           this->matches[i].producer_var->data.patch))
> +      const ir_variable *var;
> +      const glsl_type *type;
> +      bool is_vertex_input = false;
> +      if (matches[i].consumer_var) {
> +         var = matches[i].consumer_var;
> +         type = get_varying_type(var, consumer_stage);
> +         is_vertex_input = false;
This is not required. is_vertex_input is already initialized to false.
> +         if (consumer_stage == MESA_SHADER_VERTEX)
> +            is_vertex_input = true;
> +      } else {
> +         var = matches[i].producer_var;
> +         type = get_varying_type(var, producer_stage);
> +      }
> +
> +      if (var->data.patch)
>           location = &generic_patch_location;
>
>        /* Advance to the next slot if this varying has a different packing
> @@ -1094,9 +1108,45 @@ varying_matches::assign_locations(uint64_t reserved_slots, bool separate_shader)
>            != this->matches[i].packing_class) {
>           *location = ALIGN(*location, 4);
>        }
> -      while ((*location < MAX_VARYING * 4u) &&
> -            (reserved_slots & (1u << *location / 4u))) {
> -         *location = ALIGN(*location + 1, 4);
> +
> +      unsigned num_elements =  type->count_attribute_slots(is_vertex_input);
> +      unsigned slot_end = this->disable_varying_packing ? 4 :
> +         type->without_array()->vector_elements;
> +      slot_end += *location - 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
> +       * the varying between an explicit location. For now just let the user
> +       * 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 & (1u << *location / 4u) ||
> +                 (reserved_slots & (1u << slot_end / 4u))))) {
> +
> +            *location = ALIGN(*location + 1, 4);
> +            slot_end = *location;
> +
> +            /* reset the counter and try again */
> +            j = 0;
> +         }
> +
> +         /* Increase the slot to make sure there is enough room for next
> +          * array element.
> +          */
> +         if (this->disable_varying_packing)
> +            slot_end += 4;
> +         else
> +            slot_end += type->without_array()->vector_elements;
> +      }
> +
> +      if (!var->data.patch && *location >= 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 "
> +                      "using an explicit location for arrays and structs.",
> +                      var->name);
>        }
>
>        this->matches[i].generic_location = *location;
> @@ -1484,8 +1534,14 @@ reserved_varying_slot(struct gl_shader *stage, ir_variable_mode io_mode)
>           continue;
>
>        var_slot = var->data.location - VARYING_SLOT_VAR0;
> -      if (var_slot >= 0 && var_slot < MAX_VARYING)
> -         slots |= 1u << var_slot;
> +
> +      unsigned num_elements = get_varying_type(var, stage->Stage)
> +         ->count_attribute_slots(stage->Stage == MESA_SHADER_VERTEX);
> +      for (unsigned i = 0; i < num_elements; i++) {
> +         if (var_slot >= 0 && var_slot < MAX_VARYING)
> +            slots |= 1u << var_slot;
> +         var_slot += 1;
> +      }
>     }
>
>     return slots;
> @@ -1671,7 +1727,7 @@ assign_varying_locations(struct gl_context *ctx,
>        reserved_varying_slot(producer, ir_var_shader_out) |
>        reserved_varying_slot(consumer, ir_var_shader_in);
>
> -   const unsigned slots_used = matches.assign_locations(reserved_slots,
> +   const unsigned slots_used = matches.assign_locations(prog, reserved_slots,
>                                                          prog->SeparateShader);
>     matches.store_locations();
>
> --
> 2.4.3
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Acked-by: Anuj Phogat <anuj.phogat at gmail.com>


More information about the mesa-dev mailing list