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

[Martin Peres]

On 13/04/15 11:08, Martin Peres wrote:
>
> 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.

Actually, the last stage is not FS, it is CS. So first == last does not 
have to be true when compute shaders become available. You should keep 
the condition.
>
> 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];
>>>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev