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

[Tapani Pälli]

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.

// Tapani