[Mesa-dev] [PATCH v2] glsl: fix consumer_stage restriction to separate shader objects

[Tapani Pälli]

On 01/26/2016 05:59 AM, Timothy Arceri wrote:
> On Tue, 2016-01-19 at 07:20 +0100, Samuel Iglesias Gonsálvez wrote:
>> Commit 781d278 did not restrict consumer_stage only to separate
>> shader
>> objects, which is when we don't know if the consumer stage would be a
>> fragment shader added later. In normal programs, when
>> consumer_stage == -1, it is because they are not consumed.
>>
>> Fixes 4 piglit regressions added by commit 781d278 in radeonsi:
>>
>> arb_gpu_shader_fp64-double-gettransformfeedbackvarying
>> arb_gpu_shader_fp64-tf-interleaved
>> arb_gpu_shader_fp64-tf-interleaved-aligned
>> arb_gpu_shader_fp64-tf-separate
> But what happens if you change these tests to also make use of SSO? I'm
> assuming they would fall over again. That's not saying this patch isn't
> correct but that the original likely wasn't a full solution.

IMO that is a different problem. SSO needs some bigger changes, like 
moving packing to happen only later when both consumer and producer are 
known.

> It looks a lot like the issue I'm seeing with this [1] workaround for
> SSO. Transform feedback depends on packing, it seems any change in
> rules in the varying location assignment code should also be reflected
> in the transform feedback code, I don't know the correct fix yet but
> more piglit tests is probably the first step.
>
> [1] http://lists.freedesktop.org/archives/mesa-dev/2016-January/105764.
> html
>
>> v2:
>> - Simplify condition expression (Timothy).
>>
>> Signed-off-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com>
>> Tested-by: Michel Dänzer <michel.daenzer at amd.com>
>> ---
>>   src/glsl/link_varyings.cpp | 15 +++++++++------
>>   1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/glsl/link_varyings.cpp b/src/glsl/link_varyings.cpp
>> index 09f80d0..a27589d 100644
>> --- a/src/glsl/link_varyings.cpp
>> +++ b/src/glsl/link_varyings.cpp
>> @@ -824,7 +824,8 @@ public:
>>                      gl_shader_stage producer_stage,
>>                      gl_shader_stage consumer_stage);
>>      ~varying_matches();
>> -   void record(ir_variable *producer_var, ir_variable
>> *consumer_var);
>> +   void record(ir_variable *producer_var, ir_variable *consumer_var,
>> +               bool separate_shader);
>>      unsigned assign_locations(struct gl_shader_program *prog,
>>                                uint64_t reserved_slots, bool
>> separate_shader);
>>      void store_locations() const;
>> @@ -952,7 +953,8 @@ varying_matches::~varying_matches()
>>    * rendering.
>>    */
>>   void
>> -varying_matches::record(ir_variable *producer_var, ir_variable
>> *consumer_var)
>> +varying_matches::record(ir_variable *producer_var, ir_variable
>> *consumer_var,
>> +                        bool separate_shader)
>>   {
>>      assert(producer_var != NULL || consumer_var != NULL);
>>   
>> @@ -968,7 +970,8 @@ varying_matches::record(ir_variable
>> *producer_var, ir_variable *consumer_var)
>>      }
>>   
>>      if ((consumer_var == NULL && producer_var->type-
>>> contains_integer()) ||
>> -       (consumer_stage != -1 && consumer_stage !=
>> MESA_SHADER_FRAGMENT)) {
>> +       (consumer_stage != MESA_SHADER_FRAGMENT &&
>> +        (!separate_shader || consumer_stage != -1))) {
>
>
>
>>         /* Since this varying is not being consumed by the fragment
>> shader, its
>>          * interpolation type varying cannot possibly affect
>> rendering.
>>          * Also, this variable is non-flat and is (or contains) an
>> integer.
>> @@ -1667,7 +1670,7 @@ assign_varying_locations(struct gl_context
>> *ctx,
>>             */
>>            if (input_var || (prog->SeparateShader && consumer == NULL)
>> ||
>>                producer->Type == GL_TESS_CONTROL_SHADER) {
>> -            matches.record(output_var, input_var);
>> +            matches.record(output_var, input_var, prog-
>>> SeparateShader);
>>            }
>>   
>>            /* Only stream 0 outputs can be consumed in the next stage
>> */
>> @@ -1692,7 +1695,7 @@ assign_varying_locations(struct gl_context
>> *ctx,
>>                (input_var->data.mode != ir_var_shader_in))
>>               continue;
>>   
>> -         matches.record(NULL, input_var);
>> +         matches.record(NULL, input_var, prog->SeparateShader);
>>         }
>>      }
>>   
>> @@ -1711,7 +1714,7 @@ assign_varying_locations(struct gl_context
>> *ctx,
>>         }
>>   
>>         if (matched_candidate->toplevel_var-
>>> data.is_unmatched_generic_inout)
>> -         matches.record(matched_candidate->toplevel_var, NULL);
>> +         matches.record(matched_candidate->toplevel_var, NULL, prog-
>>> SeparateShader);
>>      }
>>   
>>      const uint64_t reserved_slots =
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev