[Mesa-dev] [PATCH 5/8] glsl: disable varying packing for varying used by interpolateAt*

[Timothy Arceri]

Currently the NIR backends depend on GLSL IR copy propagation to
fix up the interpolateAt* function params after varying packing
changes the shader input to a global. It's possible copy propagation
might not always do what we need it too, and we also shouldn't
depend on optimisations to do this type of thing for us.

I'm not sure if the same is true for TGSI, but the following
commit should re-enable packing for most cases in a safer way,
so we just disable it everywhere.

No change in shader-db for i965 (BDW)
---
 src/compiler/glsl/ast_function.cpp          |  2 ++
 src/compiler/glsl/link_varyings.cpp         | 17 +++++++++++++----
 src/compiler/glsl/lower_packed_varyings.cpp |  7 ++++---
 3 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/src/compiler/glsl/ast_function.cpp b/src/compiler/glsl/ast_function.cpp
index 0665e0c..2d96274 100644
--- a/src/compiler/glsl/ast_function.cpp
+++ b/src/compiler/glsl/ast_function.cpp
@@ -228,20 +228,22 @@ verify_parameter_modes(_mesa_glsl_parse_state *state,
             val = ((ir_dereference_array *)val)->array;
          }
 
          if (!val->as_dereference_variable() ||
              val->variable_referenced()->data.mode != ir_var_shader_in) {
             _mesa_glsl_error(&loc, state,
                              "parameter `%s` must be a shader input",
                              formal->name);
             return false;
          }
+
+         val->variable_referenced()->data.must_be_shader_input = 1;
       }
 
       /* Verify that 'out' and 'inout' actual parameters are lvalues. */
       if (formal->data.mode == ir_var_function_out
           || formal->data.mode == ir_var_function_inout) {
          const char *mode = NULL;
          switch (formal->data.mode) {
          case ir_var_function_out:   mode = "out";   break;
          case ir_var_function_inout: mode = "inout"; break;
          default:                    assert(false);  break;
diff --git a/src/compiler/glsl/link_varyings.cpp b/src/compiler/glsl/link_varyings.cpp
index f0df3d6..37297be 100644
--- a/src/compiler/glsl/link_varyings.cpp
+++ b/src/compiler/glsl/link_varyings.cpp
@@ -1454,31 +1454,38 @@ varying_matches::record(ir_variable *producer_var, ir_variable *consumer_var)
     *
     * This text was also in at least revison 7 of the 4.40 spec but is no
     * longer in revision 9 and not in the 4.50 spec.
     */
    const ir_variable *const var = (consumer_var != NULL)
       ? consumer_var : producer_var;
    const gl_shader_stage stage = (consumer_var != NULL)
       ? consumer_stage : producer_stage;
    const glsl_type *type = get_varying_type(var, stage);
 
+   if (producer_var && consumer_var &&
+       consumer_var->data.must_be_shader_input) {
+      producer_var->data.must_be_shader_input = 1;
+   }
+
    this->matches[this->num_matches].packing_class
       = this->compute_packing_class(var);
    this->matches[this->num_matches].packing_order
       = this->compute_packing_order(var);
-   if (this->disable_varying_packing && !is_varying_packing_safe(type, var)) {
+   if ((this->disable_varying_packing && !is_varying_packing_safe(type, var)) ||
+       var->data.must_be_shader_input) {
       unsigned slots = type->count_attribute_slots(false);
       this->matches[this->num_matches].num_components = slots * 4;
    } else {
       this->matches[this->num_matches].num_components
          = type->component_slots();
    }
+
    this->matches[this->num_matches].producer_var = producer_var;
    this->matches[this->num_matches].consumer_var = consumer_var;
    this->num_matches++;
    if (producer_var)
       producer_var->data.is_unmatched_generic_inout = 0;
    if (consumer_var)
       consumer_var->data.is_unmatched_generic_inout = 0;
 }
 
 
@@ -1537,21 +1544,22 @@ 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 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
        * when packing is disabled. Note we don't advance to the next slot if
        * we can pack varyings together that are only used for transform
        * feedback.
        */
-      if ((this->disable_varying_packing &&
+      if (var->data.must_be_shader_input ||
+          (this->disable_varying_packing &&
            !(previous_var_xfb_only && var->data.is_xfb_only)) ||
           (i > 0 && this->matches[i - 1].packing_class
           != this->matches[i].packing_class )) {
          *location = ALIGN(*location, 4);
       }
 
       previous_var_xfb_only = var->data.is_xfb_only;
 
       /* The number of components taken up by this variable. For vertex shader
        * inputs, we use the number of slots * 4, as they have different
@@ -1653,22 +1661,23 @@ varying_matches::compute_packing_class(const ir_variable *var)
     * - varyings of base type "int" and "uint" must use the "flat"
     *   interpolation type, which can only occur in GLSL 1.30 and above.
     *
     * - On platforms that support GLSL 1.30 and above, lower_packed_varyings
     *   can store flat floats as ints without losing any information (using
     *   the ir_unop_bitcast_* opcodes).
     *
     * Therefore, the packing class depends only on the interpolation type.
     */
    unsigned packing_class = var->data.centroid | (var->data.sample << 1) |
-                            (var->data.patch << 2);
-   packing_class *= 4;
+                            (var->data.patch << 2) |
+                            (var->data.must_be_shader_input << 3);
+   packing_class *= 8;
    packing_class += var->is_interpolation_flat()
       ? unsigned(INTERP_MODE_FLAT) : var->data.interpolation;
    return packing_class;
 }
 
 
 /**
  * Compute the "packing order" of the given varying.  This is a sort key we
  * use to determine when to attempt to pack the given varying relative to
  * other varyings in the same packing class.
diff --git a/src/compiler/glsl/lower_packed_varyings.cpp b/src/compiler/glsl/lower_packed_varyings.cpp
index 13f7e5b..b1a3b49 100644
--- a/src/compiler/glsl/lower_packed_varyings.cpp
+++ b/src/compiler/glsl/lower_packed_varyings.cpp
@@ -735,24 +735,25 @@ lower_packed_varyings_visitor::get_packed_varying_deref(
        */
       ir_constant *constant = new(this->mem_ctx) ir_constant(vertex_index);
       deref = new(this->mem_ctx) ir_dereference_array(deref, constant);
    }
    return deref;
 }
 
 bool
 lower_packed_varyings_visitor::needs_lowering(ir_variable *var)
 {
-   /* Things composed of vec4's and varyings with explicitly assigned
-    * locations don't need lowering.  Everything else does.
+   /* Things composed of vec4's, varyings with explicitly assigned
+    * locations or varyings marked as must_be_shader_input (which might be used
+    * by interpolateAt* functions) shouldn't be lowered. Everything else can be.
     */
-   if (var->data.explicit_location)
+   if (var->data.explicit_location || var->data.must_be_shader_input)
       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 has the same interpolation and therefore asre safe to pack.
     */
    const glsl_type *type = var->type;
    if (disable_varying_packing && !var->data.is_xfb_only &&
        !((type->is_array() || type->is_record() || type->is_matrix()) &&
-- 
2.9.3