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

[Samuel Iglesias Gonsálvez]

On Fri, 2016-01-15 at 21:29 +1100, Timothy Arceri wrote:
> On Fri, 2016-01-15 at 10:04 +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
> > 
> > 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..96f5946 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) ||
> > !separate_shader))) {
> 
> 
> I think you can just make the above line:
> 
> (!separate_shader || consumer_stage != -1)
> 

You are right :-) I am going to change it locally.

Sam

> 
> >        /* 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 =
>