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

[Martin Peres]

On 13/04/15 14:15, Tapani Pälli wrote:
>
>
> On 04/13/2015 11:18 AM, Martin Peres wrote:
>>
>>
>> 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.
>
> I'm OK with both keeping or removing it. For me it looks like this 
> function might need quite a bit of changes here and there with CS.

ACK, your call.
>
>>> 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
>>