glsl/linker: add DisableTransformFeedbackPacking workaround

Some drivers (e.g. Panfrost) don't support packing of varyings when
used for transform feedback. This new constant ensures that any
varying used for xfb is aligned at the start of a slot and won't be
packed with other varyings.

Scenarios where transform feedback declarations are related to an
array element or a struct member will be handled in a subsequent
patch.

Signed-off-by: Louis-Francis Ratté-Boulianne <[email protected]>
Signed-off-by: Tomeu Vizoso <[email protected]> (Fix order of arguments to varying_matches())
Reviewed-by: Alyssa Rosenzweig <[email protected]>
Acked-by: Daniel Stone <[email protected]>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/2433>

5 files changed, 115 insertions, 19 deletions

diff --git a/src/compiler/glsl/ir.h b/src/compiler/glsl/ir.h
index 6d79d42af45..cd1c53c29e7 100644
--- a/src/compiler/glsl/ir.h
+++ b/src/compiler/glsl/ir.h
@@ -767,6 +767,13 @@ public:
       unsigned is_unmatched_generic_inout:1;
 
       /**
+       * Is this varying used by transform feedback?
+       *
+       * This is used by the linker to decide if it's safe to pack the varying.
+       */
+      unsigned is_xfb:1;
+
+      /**
        * Is this varying used only by transform feedback?
        *
        * This is used by the linker to decide if its safe to pack the varying.
diff --git a/src/compiler/glsl/ir_optimization.h b/src/compiler/glsl/ir_optimization.h
index cd0be38e56f..11695c5aba2 100644
--- a/src/compiler/glsl/ir_optimization.h
+++ b/src/compiler/glsl/ir_optimization.h
@@ -156,7 +156,9 @@ void lower_packed_varyings(void *mem_ctx,
                            ir_variable_mode mode,
                            unsigned gs_input_vertices,
                            gl_linked_shader *shader,
-                           bool disable_varying_packing, bool xfb_enabled);
+                           bool disable_varying_packing,
+                           bool disable_xfb_packing,
+                           bool xfb_enabled);
 bool lower_vector_insert(exec_list *instructions, bool lower_nonconstant_index);
 bool lower_vector_derefs(gl_linked_shader *shader);
 void lower_named_interface_blocks(void *mem_ctx, gl_linked_shader *shader);
diff --git a/src/compiler/glsl/link_varyings.cpp b/src/compiler/glsl/link_varyings.cpp
index 0868d1504db..4b63f7693f6 100644
--- a/src/compiler/glsl/link_varyings.cpp
+++ b/src/compiler/glsl/link_varyings.cpp
@@ -1590,7 +1590,9 @@ namespace {
 class varying_matches
 {
 public:
-   varying_matches(bool disable_varying_packing, bool xfb_enabled,
+   varying_matches(bool disable_varying_packing,
+                   bool disable_xfb_packing,
+                   bool xfb_enabled,
                    bool enhanced_layouts_enabled,
                    gl_shader_stage producer_stage,
                    gl_shader_stage consumer_stage);
@@ -1617,10 +1619,16 @@ private:
    const bool disable_varying_packing;
 
    /**
+    * If true, this driver disables packing for varyings used by transform
+    * feedback.
+    */
+   const bool disable_xfb_packing;
+
+   /**
     * If true, this driver has transform feedback enabled. The transform
-    * feedback code requires at least some packing be done even when varying
-    * packing is disabled, fortunately where transform feedback requires
-    * packing it's safe to override the disabled setting. See
+    * feedback code usually requires at least some packing be done even
+    * when varying packing is disabled, fortunately where transform feedback
+    * requires packing it's safe to override the disabled setting. See
     * is_varying_packing_safe().
     */
    const bool xfb_enabled;
@@ -1647,6 +1655,7 @@ private:
    static packing_order_enum compute_packing_order(const ir_variable *var);
    static int match_comparator(const void *x_generic, const void *y_generic);
    static int xfb_comparator(const void *x_generic, const void *y_generic);
+   static int not_xfb_comparator(const void *x_generic, const void *y_generic);
 
    /**
     * Structure recording the relationship between a single producer output
@@ -1702,11 +1711,13 @@ private:
 } /* anonymous namespace */
 
 varying_matches::varying_matches(bool disable_varying_packing,
+                                 bool disable_xfb_packing,
                                  bool xfb_enabled,
                                  bool enhanced_layouts_enabled,
                                  gl_shader_stage producer_stage,
                                  gl_shader_stage consumer_stage)
    : disable_varying_packing(disable_varying_packing),
+     disable_xfb_packing(disable_xfb_packing),
      xfb_enabled(xfb_enabled),
      enhanced_layouts_enabled(enhanced_layouts_enabled),
      producer_stage(producer_stage),
@@ -1785,6 +1796,7 @@ varying_matches::record(ir_variable *producer_var, ir_variable *consumer_var)
        producer_var->type->contains_double());
 
    if (!disable_varying_packing &&
+       (!disable_xfb_packing || producer_var  == NULL || !producer_var->data.is_xfb) &&
        (needs_flat_qualifier ||
         (consumer_stage != MESA_SHADER_NONE && consumer_stage != MESA_SHADER_FRAGMENT))) {
       /* Since this varying is not being consumed by the fragment shader, its
@@ -1850,6 +1862,7 @@ varying_matches::record(ir_variable *producer_var, ir_variable *consumer_var)
    this->matches[this->num_matches].packing_order
       = this->compute_packing_order(var);
    if ((this->disable_varying_packing && !is_varying_packing_safe(type, var)) ||
+       (this->disable_xfb_packing && var->data.is_xfb) ||
        var->data.must_be_shader_input) {
       unsigned slots = type->count_attribute_slots(false);
       this->matches[this->num_matches].num_components = slots * 4;
@@ -1890,19 +1903,29 @@ varying_matches::assign_locations(struct gl_shader_program *prog,
     * When packing is disabled the sort orders varyings used by transform
     * feedback first, but also depends on *undefined behaviour* of qsort to
     * reverse the order of the varyings. See: xfb_comparator().
+    *
+    * If packing is only disabled for xfb varyings (mutually exclusive with
+    * disable_varying_packing), we then group varyings depending on if they
+    * are captured for transform feedback. The same *undefined behaviour* is
+    * taken advantage of.
     */
-   if (!this->disable_varying_packing) {
-      /* Sort varying matches into an order that makes them easy to pack. */
-      qsort(this->matches, this->num_matches, sizeof(*this->matches),
-            &varying_matches::match_comparator);
-   } else {
+   if (this->disable_varying_packing) {
       /* Only sort varyings that are only used by transform feedback. */
       qsort(this->matches, this->num_matches, sizeof(*this->matches),
             &varying_matches::xfb_comparator);
+   } else if (this->disable_xfb_packing) {
+      /* Only sort varyings that are NOT used by transform feedback. */
+      qsort(this->matches, this->num_matches, sizeof(*this->matches),
+            &varying_matches::not_xfb_comparator);
+   } else {
+      /* Sort varying matches into an order that makes them easy to pack. */
+      qsort(this->matches, this->num_matches, sizeof(*this->matches),
+            &varying_matches::match_comparator);
    }
 
    unsigned generic_location = 0;
    unsigned generic_patch_location = MAX_VARYING*4;
+   bool previous_var_xfb = false;
    bool previous_var_xfb_only = false;
    unsigned previous_packing_class = ~0u;
 
@@ -1939,6 +1962,9 @@ varying_matches::assign_locations(struct gl_shader_program *prog,
        * class than the previous one, and we're not already on a slot
        * boundary.
        *
+       * Also advance if varying packing is disabled for transform feedback,
+       * and previous or current varying is used for transform feedback.
+       *
        * Also advance to the next slot if packing is disabled. This makes sure
        * we don't assign varyings the same locations which is possible
        * because we still pack individual arrays, records and matrices even
@@ -1947,6 +1973,8 @@ varying_matches::assign_locations(struct gl_shader_program *prog,
        * feedback.
        */
       if (var->data.must_be_shader_input ||
+          (this->disable_xfb_packing &&
+           (previous_var_xfb || var->data.is_xfb)) ||
           (this->disable_varying_packing &&
            !(previous_var_xfb_only && var->data.is_xfb_only)) ||
           (previous_packing_class != this->matches[i].packing_class) ||
@@ -1955,6 +1983,7 @@ varying_matches::assign_locations(struct gl_shader_program *prog,
          *location = ALIGN(*location, 4);
       }
 
+      previous_var_xfb = var->data.is_xfb;
       previous_var_xfb_only = var->data.is_xfb_only;
       previous_packing_class = this->matches[i].packing_class;
 
@@ -2212,6 +2241,32 @@ varying_matches::xfb_comparator(const void *x_generic, const void *y_generic)
 
 
 /**
+ * Comparison function passed to qsort() to sort varyings NOT used by
+ * transform feedback when packing of xfb varyings is disabled.
+ */
+int
+varying_matches::not_xfb_comparator(const void *x_generic, const void *y_generic)
+{
+   const match *x = (const match *) x_generic;
+
+   if (x->producer_var != NULL && !x->producer_var->data.is_xfb)
+      return match_comparator(x_generic, y_generic);
+
+   /* FIXME: When the comparator returns 0 it means the elements being
+    * compared are equivalent. However the qsort documentation says:
+    *
+    *    "The order of equivalent elements is undefined."
+    *
+    * In practice the sort ends up reversing the order of the varyings which
+    * means locations are also assigned in this reversed order and happens to
+    * be what we want. This is also whats happening in
+    * varying_matches::match_comparator().
+    */
+   return 0;
+}
+
+
+/**
  * Is the given variable a varying variable to be counted against the
  * limit in ctx->Const.MaxVarying?
  * This includes variables such as texcoords, colors and generic
@@ -2558,11 +2613,17 @@ assign_varying_locations(struct gl_context *ctx,
 
    /* Transform feedback code assumes varying arrays are packed, so if the
     * driver has disabled varying packing, make sure to at least enable
-    * packing required by transform feedback.
+    * packing required by transform feedback. See below for exception.
     */
    bool xfb_enabled =
       ctx->Extensions.EXT_transform_feedback && !unpackable_tess;
 
+   /* Some drivers actually requires packing to be explicitly disabled
+    * for varyings used by transform feedback.
+    */
+   bool disable_xfb_packing =
+      ctx->Const.DisableTransformFeedbackPacking;
+
    /* Disable packing on outward facing interfaces for SSO because in ES we
     * need to retain the unpacked varying information for draw time
     * validation.
@@ -2577,7 +2638,9 @@ assign_varying_locations(struct gl_context *ctx,
    if (prog->SeparateShader && (producer == NULL || consumer == NULL))
       disable_varying_packing = true;
 
-   varying_matches matches(disable_varying_packing, xfb_enabled,
+   varying_matches matches(disable_varying_packing,
+                           disable_xfb_packing,
+                           xfb_enabled,
                            ctx->Extensions.ARB_enhanced_layouts,
                            producer ? producer->Stage : MESA_SHADER_NONE,
                            consumer ? consumer->Stage : MESA_SHADER_NONE);
@@ -2732,8 +2795,10 @@ assign_varying_locations(struct gl_context *ctx,
                                     consumer_inputs,
                                     consumer_interface_inputs,
                                     consumer_inputs_with_locations);
-      if (input_var)
+      if (input_var) {
+         input_var->data.is_xfb = 1;
          input_var->data.always_active_io = 1;
+      }
 
       if (matched_candidate->toplevel_var->data.is_unmatched_generic_inout) {
          matched_candidate->toplevel_var->data.is_xfb_only = 1;
@@ -2804,13 +2869,13 @@ assign_varying_locations(struct gl_context *ctx,
    if (producer) {
       lower_packed_varyings(mem_ctx, slots_used, components, ir_var_shader_out,
                             0, producer, disable_varying_packing,
-                            xfb_enabled);
+                            disable_xfb_packing, xfb_enabled);
    }
 
    if (consumer) {
       lower_packed_varyings(mem_ctx, slots_used, components, ir_var_shader_in,
-                            consumer_vertices, consumer,
-                            disable_varying_packing, xfb_enabled);
+                            consumer_vertices, consumer, disable_varying_packing,
+                            disable_xfb_packing, xfb_enabled);
    }
 
    return true;
diff --git a/src/compiler/glsl/lower_packed_varyings.cpp b/src/compiler/glsl/lower_packed_varyings.cpp
index d15a45960b3..9c418ebae63 100644
--- a/src/compiler/glsl/lower_packed_varyings.cpp
+++ b/src/compiler/glsl/lower_packed_varyings.cpp
@@ -173,6 +173,7 @@ public:
                                  exec_list *out_instructions,
                                  exec_list *out_variables,
                                  bool disable_varying_packing,
+                                 bool disable_xfb_packing,
                                  bool xfb_enabled);
 
    void run(struct gl_linked_shader *shader);
@@ -240,6 +241,7 @@ private:
    exec_list *out_variables;
 
    bool disable_varying_packing;
+   bool disable_xfb_packing;
    bool xfb_enabled;
 };
 
@@ -250,7 +252,7 @@ lower_packed_varyings_visitor::lower_packed_varyings_visitor(
       ir_variable_mode mode,
       unsigned gs_input_vertices, exec_list *out_instructions,
       exec_list *out_variables, bool disable_varying_packing,
-      bool xfb_enabled)
+      bool disable_xfb_packing, bool xfb_enabled)
    : mem_ctx(mem_ctx),
      locations_used(locations_used),
      components(components),
@@ -262,6 +264,7 @@ lower_packed_varyings_visitor::lower_packed_varyings_visitor(
      out_instructions(out_instructions),
      out_variables(out_variables),
      disable_varying_packing(disable_varying_packing),
+     disable_xfb_packing(disable_xfb_packing),
      xfb_enabled(xfb_enabled)
 {
 }
@@ -769,12 +772,21 @@ lower_packed_varyings_visitor::needs_lowering(ir_variable *var)
    if (var->data.explicit_location || var->data.must_be_shader_input)
       return false;
 
+   const glsl_type *type = var->type;
+
+   /* Some drivers (e.g. panfrost) don't support packing of transform
+    * feedback varyings.
+    */
+   if (disable_xfb_packing && var->data.is_xfb &&
+       !(type->is_array() || type->is_struct() || type->is_matrix()) &&
+       xfb_enabled)
+      return false;
+
    /* Override disable_varying_packing if the var is only used by transform
     * feedback. Also override it if transform feedback is enabled and the
     * variable is an array, struct or matrix as the elements of these types
     * will always have the same interpolation and therefore are safe to pack.
     */
-   const glsl_type *type = var->type;
    if (disable_varying_packing && !var->data.is_xfb_only &&
        !((type->is_array() || type->is_struct() || type->is_matrix()) &&
          xfb_enabled))
@@ -874,7 +886,7 @@ lower_packed_varyings(void *mem_ctx, unsigned locations_used,
                       const uint8_t *components,
                       ir_variable_mode mode, unsigned gs_input_vertices,
                       gl_linked_shader *shader, bool disable_varying_packing,
-                      bool xfb_enabled)
+                      bool disable_xfb_packing, bool xfb_enabled)
 {
    exec_list *instructions = shader->ir;
    ir_function *main_func = shader->symbols->get_function("main");
@@ -890,6 +902,7 @@ lower_packed_varyings(void *mem_ctx, unsigned locations_used,
                                          &new_instructions,
                                          &new_variables,
                                          disable_varying_packing,
+                                         disable_xfb_packing,
                                          xfb_enabled);
    visitor.run(shader);
    if (mode == ir_var_shader_out) {
diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
index 6d18d2f5de5..91fc71cfcfd 100644
--- a/src/mesa/main/mtypes.h
+++ b/src/mesa/main/mtypes.h
@@ -3975,6 +3975,15 @@ struct gl_constants
    GLboolean DisableVaryingPacking;
 
    /**
+    * Disable varying packing if used for transform feedback.  This is needed
+    * for some drivers (e.g. Panfrost) where transform feedback requires
+    * unpacked varyings.
+    *
+    * This variable is mutually exlusive with DisableVaryingPacking.
+    */
+   GLboolean DisableTransformFeedbackPacking;
+
+   /**
     * UBOs and SSBOs can be packed tightly by the OpenGL implementation when
     * layout is set as shared (the default) or packed. However most Mesa drivers
     * just use STD140 for these layouts. This flag allows drivers to use STD430