[Mesa-dev] [PATCH 5/6] glsl: don't sort varying in separate shader mode

[Timothy Arceri]

On Mon, 2015-11-30 at 14:31 +0200, Tapani Pälli wrote:
> Reviewed-by: Tapani Pälli <tapani.palli at intel.com>
> 
> On 11/25/2015 11:54 AM, Timothy Arceri wrote:
> > From: Gregory Hainaut <gregory.hainaut at gmail.com>
> > 
> > This fixes an issue where the addition of the FLAT qualifier in
> > varying_matches::record() can break the expected varying order.
> > 
> > It also avoids a future issue with the relaxing of interpolation
> > qualifier matching constraints in GLSL 4.50.
> > 
> > V2: (by Timothy Arceri)
> > * reworked comment slightly
> > 
> > Signed-off-by: Gregory Hainaut <gregory.hainaut at gmail.com>
> > Reviewed-by: Timothy Arceri <timothy.arceri at collabora.com>
> > ---
> >   src/glsl/link_varyings.cpp | 38 ++++++++++++++++++++++++++++++++-
> > -----
> >   1 file changed, 32 insertions(+), 6 deletions(-)
> > 
> > diff --git a/src/glsl/link_varyings.cpp
> > b/src/glsl/link_varyings.cpp
> > index ac2755f..71750d1 100644
> > --- a/src/glsl/link_varyings.cpp
> > +++ b/src/glsl/link_varyings.cpp
> > @@ -766,7 +766,7 @@ public:
> >                      gl_shader_stage consumer_stage);
> >      ~varying_matches();
> >      void record(ir_variable *producer_var, ir_variable
> > *consumer_var);
> > -   unsigned assign_locations(uint64_t reserved_slots);
> > +   unsigned assign_locations(uint64_t reserved_slots, bool
> > separate_shader);
> >      void store_locations() const;
> > 
> >   private:
> > @@ -988,11 +988,36 @@ varying_matches::record(ir_variable
> > *producer_var, ir_variable *consumer_var)
> >    * passed to varying_matches::record().
> >    */
> >   unsigned
> > -varying_matches::assign_locations(uint64_t reserved_slots)
> > +varying_matches::assign_locations(uint64_t reserved_slots, bool
> > separate_shader)
> >   {
> > -   /* Sort varying matches into an order that makes them easy to
> > pack. */
> > -   qsort(this->matches, this->num_matches, sizeof(*this->matches),
> > -         &varying_matches::match_comparator);
> > +   /* We disable varying sorting for separate shader programs for
> > the
> > +    * following reasons:
> > +    *
> > +    * 1/ All programs must sort the code in the same order to
> > guarantee the
> > +    *    interface matching. However varying_matches::record()
> > will change the
> > +    *    interpolation qualifier of some stages.
> > +    *
> > +    * 2/ GLSL version 4.50 removes the matching constrain on the
> > interpolation
> > +    *    qualifier.
> > +    *
> > +    * From Section 4.5 (Interpolation Qualifiers) of the GLSL 4.40
> > spec:
> > +    *
> > +    *    "The type and presence of interpolation qualifiers of
> > variables with
> > +    *    the same name declared in all linked shaders for the same
> > cross-stage
> > +    *    interface must match, otherwise the link command will
> > fail.
> > +    *
> > +    *    When comparing an output from one stage to an input of a
> > subsequent
> > +    *    stage, the input and output don't match if their
> > interpolation
> > +    *    qualifiers (or lack thereof) are not the same."
> > +    *
> > +    *    "It is a link-time error if, within the same stage, the
> > interpolation
> > +    *    qualifiers of variables of the same name do not match."
> > +    */
> > +   if (!separate_shader) {
> > +      /* Sort varying matches into an order that makes them easy
> > to pack. */
> > +      qsort(this->matches, this->num_matches, sizeof(*this
> > ->matches),
> > +            &varying_matches::match_comparator);
> > +   }
> > 
> >      unsigned generic_location = 0;
> >      unsigned generic_patch_location = MAX_VARYING*4;
> > @@ -1592,7 +1617,8 @@ assign_varying_locations(struct gl_context
> > *ctx,
> >         reserved_varying_slot(producer, ir_var_shader_out) |
> >         reserved_varying_slot(consumer, ir_var_shader_in);
> > 
> > -   const unsigned slots_used =
> > matches.assign_locations(reserved_slots);
> > +   const unsigned slots_used =
> > matches.assign_locations(reserved_slots,
> > +                                                        prog
> > ->SeparateShader);
> >      matches.store_locations();
> > 
> >      for (unsigned i = 0; i < num_tfeedback_decls; ++i) {
> > 

I haven't figured out why yet but this patch breaks transform feedback
when the last stage is a SSO (which there is currently no piglit tests
for).