[Mesa-dev] [PATCH] glsl: restrict consumer stage condition to modify interpolation type

[Tapani Pälli]

On 01/14/2016 09:38 AM, Samuel Iglesias Gonsálvez wrote:
> Only modify interpolation type for integer-based varyings or when the
> consumer is known and different than fragment shader.
>
> If we are linking separate shader programs and the consumer is unknown,
> the consumer could be added later and be a fragment shader. If we
> modify the interpolation type in this case, we could read wrong
> values in the fragment shader inputs, as shown in bug 93320.

Makes sense;
Reviewed-by: Tapani Pälli <tapani.palli at intel.com>

> Fixes the following CTS test:
>     ES31-CTS.vertex_attrib_binding.advanced-bindingUpdate
>
> Fixes the following dEQP tests:
>
> dEQP-GLES31.functional.separate_shader.random.102
> dEQP-GLES31.functional.separate_shader.random.111
> dEQP-GLES31.functional.separate_shader.random.115
> dEQP-GLES31.functional.separate_shader.random.17
> dEQP-GLES31.functional.separate_shader.random.22
> dEQP-GLES31.functional.separate_shader.random.23
> dEQP-GLES31.functional.separate_shader.random.3
> dEQP-GLES31.functional.separate_shader.random.32
> dEQP-GLES31.functional.separate_shader.random.39
> dEQP-GLES31.functional.separate_shader.random.64
> dEQP-GLES31.functional.separate_shader.random.73
> dEQP-GLES31.functional.separate_shader.random.91
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93320
> Signed-off-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com>
> ---
>
> This patch adds 2 regressions in dEQP:
>
> dEQP-GLES31.functional.separate_shader.random.49
> dEQP-GLES31.functional.separate_shader.random.106
>
> The failure is returned by validation_io() because the number of inputs
> and outputs does not match with this patch applied.

This is unfortunate but we should be able to live with this considering 
the end effect is more passing tests.

> As the interpolation type is not modified in
> varying_matches::record() when we don't know the consumer
> (consumer_stage == -1, for example in some separate shader objects), we
> don't pack them together because its packing class does not match.
>
> As a result, some output packed varyings are in different varying slots
> than the input ones. Due to that, we have a mismatch in the number of
> inputs and outputs because we don't check how many varyings we have
> inside of each varying slot ("packed:var0,var1...") nor their type.

This is quite a tricky problem. I've been trying to come up with a 
solution for this with help of 'packed_varyings' list but it's quite 
messy. I think for interface matching we should build some special 
structure beforehand to make it faster and robust, haven't just figured 
out how it could look. Maybe all packed varyings should have a pointer 
to a list where original variables (with original types and names exist)?

Any recommendations/proposals to deal with this issue greatly appreciated!

> The validation of packed varyings doesn't seem to be trivial
> and this is a different issue than the one this patch fixes.
>
>   src/glsl/link_varyings.cpp | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/src/glsl/link_varyings.cpp b/src/glsl/link_varyings.cpp
> index 7cc5880..09f80d0 100644
> --- a/src/glsl/link_varyings.cpp
> +++ b/src/glsl/link_varyings.cpp
> @@ -968,10 +968,12 @@ varying_matches::record(ir_variable *producer_var, ir_variable *consumer_var)
>      }
>
>      if ((consumer_var == NULL && producer_var->type->contains_integer()) ||
> -       consumer_stage != MESA_SHADER_FRAGMENT) {
> +       (consumer_stage != -1 && consumer_stage != MESA_SHADER_FRAGMENT)) {
>         /* Since this varying is not being consumed by the fragment shader, its
> -       * interpolation type varying cannot possibly affect rendering.  Also,
> -       * this variable is non-flat and is (or contains) an integer.
> +       * interpolation type varying cannot possibly affect rendering.
> +       * Also, this variable is non-flat and is (or contains) an integer.
> +       * If the consumer stage is unknown, don't modify the interpolation
> +       * type as it could affect rendering later with separate shaders.
>          *
>          * lower_packed_varyings requires all integer varyings to flat,
>          * regardless of where they appear.  We can trivially satisfy that
>