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

[Samuel Iglesias Gonsálvez]

On Tue, 2016-01-26 at 14:59 +1100, Timothy Arceri wrote:
> 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.
> 

I reviewed again this regressions and checked what ARB_gpu_shader_fp64
says. According to the spec, doubles don't support interpolation:

"This extension does not support interpolation of double-precision
values; doubles used as fragment shader inputs must be qualified as
"flat"."

Mesa was assigning the flat qualifier to the doubles unintentionally
before 781d278 because there was no consumer stage (the program only
has a vertex shader attached to it), so this bug would be also in SSOs
after this patch. I am going to send a patch to make explicit the
double's case.

Sam

> [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 =
>