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

[Martin Peres]

On 02/04/15 13:27, Tapani Pälli wrote:
>
>
> 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 :)

Ok, please get rid of it then, to eliminate the possible confusion with 
another local variable later on.

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

Let's keep it.

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

ACK.

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

Yes, please remove it.

With this, the patch is Reviewed-by: Martin Peres 
<martin.peres at linux.intel.com>
>
>>>         /* If the program only contains a fragment shader...
>>>          */
>>>         gl_shader *const sh = prog->_LinkedShaders[first];
>>