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

[Tapani Pälli]

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.

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