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

[Timothy Arceri]

On Mon, 2016-01-25 at 16:41 +1100, Timothy Arceri wrote:
> 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).

OK so the problem seems to be that if we don't do a sort we don't move
vec4's to the top of the list, this means they don't get assigned the
first locations which means they can end up getting split.

e.g 

If float a is assigned location 0 then vec4 b will be assigned location
0 with component 3 in location 1.

This is obviously bad to begin with and it also causes the backend to
fall over for transform feedback as its not expecting a vec4 to be
split over multiple locations.


> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev