[Mesa-dev] [PATCH v2] glsl: fix consumer_stage restriction to separate shader objects

[Timothy Arceri]

On Tue, 2016-01-19 at 07:20 +0100, Samuel Iglesias Gonsálvez wrote:
> Commit 781d278 did not restrict consumer_stage only to separate
> shader
> objects, which is when we don't know if the consumer stage would be a
> fragment shader added later. In normal programs, when
> consumer_stage == -1, it is because they are not consumed.
> 
> Fixes 4 piglit regressions added by commit 781d278 in radeonsi:
> 
> arb_gpu_shader_fp64-double-gettransformfeedbackvarying
> arb_gpu_shader_fp64-tf-interleaved
> arb_gpu_shader_fp64-tf-interleaved-aligned
> arb_gpu_shader_fp64-tf-separate

But what happens if you change these tests to also make use of SSO? I'm
assuming they would fall over again. That's not saying this patch isn't
correct but that the original likely wasn't a full solution.

It looks a lot like the issue I'm seeing with this [1] workaround for
SSO. Transform feedback depends on packing, it seems any change in
rules in the varying location assignment code should also be reflected
in the transform feedback code, I don't know the correct fix yet but
more piglit tests is probably the first step.

[1] http://lists.freedesktop.org/archives/mesa-dev/2016-January/105764.
html

> 
> v2:
> - Simplify condition expression (Timothy).
> 
> Signed-off-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com>
> Tested-by: Michel Dänzer <michel.daenzer at amd.com>
> ---
>  src/glsl/link_varyings.cpp | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/src/glsl/link_varyings.cpp b/src/glsl/link_varyings.cpp
> index 09f80d0..a27589d 100644
> --- a/src/glsl/link_varyings.cpp
> +++ b/src/glsl/link_varyings.cpp
> @@ -824,7 +824,8 @@ public:
>                     gl_shader_stage producer_stage,
>                     gl_shader_stage consumer_stage);
>     ~varying_matches();
> -   void record(ir_variable *producer_var, ir_variable
> *consumer_var);
> +   void record(ir_variable *producer_var, ir_variable *consumer_var,
> +               bool separate_shader);
>     unsigned assign_locations(struct gl_shader_program *prog,
>                               uint64_t reserved_slots, bool
> separate_shader);
>     void store_locations() const;
> @@ -952,7 +953,8 @@ varying_matches::~varying_matches()
>   * rendering.
>   */
>  void
> -varying_matches::record(ir_variable *producer_var, ir_variable
> *consumer_var)
> +varying_matches::record(ir_variable *producer_var, ir_variable
> *consumer_var,
> +                        bool separate_shader)
>  {
>     assert(producer_var != NULL || consumer_var != NULL);
>  
> @@ -968,7 +970,8 @@ varying_matches::record(ir_variable
> *producer_var, ir_variable *consumer_var)
>     }
>  
>     if ((consumer_var == NULL && producer_var->type-
> >contains_integer()) ||
> -       (consumer_stage != -1 && consumer_stage !=
> MESA_SHADER_FRAGMENT)) {
> +       (consumer_stage != MESA_SHADER_FRAGMENT &&
> +        (!separate_shader || consumer_stage != -1))) {




>        /* 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.
> @@ -1667,7 +1670,7 @@ assign_varying_locations(struct gl_context
> *ctx,
>            */
>           if (input_var || (prog->SeparateShader && consumer == NULL)
> ||
>               producer->Type == GL_TESS_CONTROL_SHADER) {
> -            matches.record(output_var, input_var);
> +            matches.record(output_var, input_var, prog-
> >SeparateShader);
>           }
>  
>           /* Only stream 0 outputs can be consumed in the next stage
> */
> @@ -1692,7 +1695,7 @@ assign_varying_locations(struct gl_context
> *ctx,
>               (input_var->data.mode != ir_var_shader_in))
>              continue;
>  
> -         matches.record(NULL, input_var);
> +         matches.record(NULL, input_var, prog->SeparateShader);
>        }
>     }
>  
> @@ -1711,7 +1714,7 @@ assign_varying_locations(struct gl_context
> *ctx,
>        }
>  
>        if (matched_candidate->toplevel_var-
> >data.is_unmatched_generic_inout)
> -         matches.record(matched_candidate->toplevel_var, NULL);
> +         matches.record(matched_candidate->toplevel_var, NULL, prog-
> >SeparateShader);
>     }
>  
>     const uint64_t reserved_slots =