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

[Tapani Pälli]

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? 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.

// Tapani