[Mesa-dev] [PATCH v2 01/24] linker: fix varying linking if SSO program has only gs and fs

[Tapani Pälli]

On 04/02/2015 12:36 PM, Martin Peres wrote:
>
>
> On 01/04/15 15:14, Tapani Pälli wrote:
>> Previously linker did not take in to account case where one would
>> have only gs and fs (with SSO), patch adds the case by refactoring
>> code around assign_varying_locations. This makes sure locations for
>> gs get populated correctly.
>>
>> This was found with some of the SSO subtests of Martin's upcoming
>> GetProgramInterfaceiv Piglit test which passes with the patch, no
>> Piglit regressions.
>>
>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>> ---
>>   src/glsl/linker.cpp | 32 +++++++++++++++++++-------------
>>   1 file changed, 19 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
>> index 85830e6..73432b2 100644
>> --- a/src/glsl/linker.cpp
>> +++ b/src/glsl/linker.cpp
>> @@ -2726,10 +2726,19 @@ link_shaders(struct gl_context *ctx, struct
>> gl_shader_program *prog)
>>         goto done;
>>      }
>> -   unsigned first;
>> -   for (first = 0; first <= MESA_SHADER_FRAGMENT; first++) {
>> -      if (prog->_LinkedShaders[first] != NULL)
>> -     break;
>> +   unsigned first, last;
>> +
>> +   first = MESA_SHADER_STAGES;
>> +   last = 0;
>> +
>> +   /* Determine first and last stage. */
>> +   for (unsigned i = 0; i < MESA_SHADER_STAGES; i++) {
>> +      struct gl_shader *sh = prog->_LinkedShaders[i];
> Why create this variable?

True, it is not really needed, even in the place from where this loop 
was copypasted from, see patch 3 in the series :)

>> +      if (!sh)
>> +         continue;
>> +      if (first == MESA_SHADER_STAGES)
>> +         first = i;
>> +      last = i;
>>      }
>>      if (num_tfeedback_decls != 0) {
>> @@ -2758,13 +2767,9 @@ link_shaders(struct gl_context *ctx, struct
>> gl_shader_program *prog)
>>       * ensures that inter-shader outputs written to in an earlier
>> stage are
>>       * eliminated if they are (transitively) not used in a later stage.
>>       */
>> -   int last, next;
>> -   for (last = MESA_SHADER_FRAGMENT; last >= 0; last--) {
>> -      if (prog->_LinkedShaders[last] != NULL)
>> -         break;
>> -   }
>> +   int next;
>
> So, the above is a cleanup for finding the first and last shader stage.
> It is however not necessary.

Yes, it is only cleanup to make 2 for loops as just one, can be dropped.

>> -   if (last >= 0 && last < MESA_SHADER_FRAGMENT) {
>> +   if (first < MESA_SHADER_FRAGMENT) {
>>         gl_shader *const sh = prog->_LinkedShaders[last];
>>         if (first == MESA_SHADER_GEOMETRY) {
>> @@ -2776,13 +2781,14 @@ link_shaders(struct gl_context *ctx, struct
>> gl_shader_program *prog)
>>             * MESA_SHADER_GEOMETRY.
>>             */
>>            if (!assign_varying_locations(ctx, mem_ctx, prog,
>> -                                       NULL, sh,
>> +                                       NULL,
>> prog->_LinkedShaders[first],
> The above change should not change anything because first == last ==
> MESA_SHADER_GEOMETRY. Please get rid of it if I am right.

You are not right. Here last can be either GS or FS. The point of this 
locations assignment call is that we do not have VS but still want to 
assign locations to GS. Then only in the very last loop in this 
function, varyings between GS->FS are checked.

>>                                          num_tfeedback_decls,
>> tfeedback_decls,
>>                                          prog->Geom.VerticesIn))
>>               goto done;
>>         }
>> -      if (num_tfeedback_decls != 0 || prog->SeparateShader) {
>> +      if (last != MESA_SHADER_FRAGMENT &&
>> +         (num_tfeedback_decls != 0 || prog->SeparateShader)) {
>>            /* There was no fragment shader, but we still have to
>> assign varying
>>             * locations for use by transform feedback.
>>             */
>> @@ -2804,7 +2810,7 @@ link_shaders(struct gl_context *ctx, struct
>> gl_shader_program *prog)
>>         while (do_dead_code(sh->ir, false))
>>            ;
>>      }
>> -   else if (first == MESA_SHADER_FRAGMENT) {
>> +   else if (first == MESA_SHADER_FRAGMENT && first == last) {
> How could first != last since fragment is the last stage anyway? Why did
> you add this test?

This can be removed, I added only to emphasize that we really do only 
have one but the comment below is enough. Will remove this.

>>         /* If the program only contains a fragment shader...
>>          */
>>         gl_shader *const sh = prog->_LinkedShaders[first];
>