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

[Timothy Arceri]

On Mon, 2016-01-25 at 13:27 +0200, Tapani Pälli wrote:
> 
> On 01/25/2016 01:14 PM, Timothy Arceri wrote:
> > On Mon, 2016-01-25 at 12:47 +0200, Tapani Pälli wrote:
> > > 
> > > On 01/25/2016 12:29 PM, Timothy Arceri wrote:
> > > > 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.
> > > 
> > > Did you try to disable varying packing?
> > 
> > No because the transform feedback code depends on packing being
> > enabled
> > so that would break things worse and I don't really feel like
> > rewriting
> > the transform feedback code :P
> 
> Ah right
> 
> > >   I believe it is broken for SSO
> > > currently. IMO packing cannot be done safely before both producer
> > > and
> > > consumer interface are known, and producer is really known only
> > > when
> > > we
> > > validate the pipeline for first time. We discussed this with
> > > Samuel
> > > some
> > > time ago related to cases where consumer interface may override
> > > producer
> > > qualifiers, in those cases packing does not work correctly.
> > 
> > Can you give an example of what you mean?
> > 
> 
> 
> IIRC the issue is that fragment shader (and maybe some other stages 
> too?) can override interpolation and auxiliary qualifiers of what's 
> given in vertex shader. With regular shader we can just override 
> producer's qualifiers before packing so that their 'packing class'
> will 
> match but with SSO we will have different 'packing class' for these
> when 
> packing (as such override cannot be done because consumer is not
> known 
> yet) so interfaces will not look the same and some variables may get 
> scattered to different locations.

Right, I think I see what you are getting at thanks.

> 
> // Tapani