On Mon, Dec 28, 2015 at 9:00 PM, Timothy Arceri <timothy.arc...@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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Acked-by: Anuj Phogat <anuj.pho...@gmail.com> _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev