[Mesa-dev] [PATCH] glsl/linker: Allow unused in blocks which are not declated on previous stage

[Vadym Shovkoplias]

I agree that this comment is obsolete now. I'll update the patch, thanks!

On Wed, Aug 22, 2018 at 12:09 PM, Alejandro Piñeiro <apinheiro at igalia.com>
wrote:

> On 21/08/18 11:42, Vadym Shovkoplias wrote:
>
> Hi Timothy, Alejandro,
>
> Thanks for the review comments!
> I'd just like to mention that the same approach is already used
> in link_varyings.cpp file in function cross_validate_outputs_to_inputs().
> Here is a code snippet:
>
> if (*input->data.used *&& !input->get_interface_type() &&
>                 !input->data.explicit_location && !prog->SeparateShader)
>                linker_error(prog,
>                             "%s shader input `%s' "
>                             "has no matching output in the previous
> stage\n",
>                             _mesa_shader_stage_to_string(consumer->Stage),
>                             input->name);
>
>
> This code is used for the same purpose to validate input and output
> variables which is also done during linking stage.
> So basically I just used the same check but for interface blocks.
>
> This was implemented some time ago in the following patch:
>
> commit 18004c338f6be8af2e36d2f54972c60136229aeb
> Author: Samuel Iglesias Gonsalvez <siglesias at igalia.com>
> Date:   Fri Nov 28 11:23:20 2014 +0100
>
>     glsl: fail when a shader's input var has not an equivalent out var in
> previous
>
>
>
> Suggest please does this mean that it is safe to use "used" field while
> linking ?
>
>
> Well, it seems so. Or at least it was already being used as that, and have
> been working since 2014. Then I think that it would be ok to use it for
> your patch, but I still think that in this case it would be needed to
> slightly tweak the comment at ir.h
>
>
>
> On Tue, Aug 21, 2018 at 12:00 PM, Alejandro Piñeiro <apinheiro at igalia.com>
> wrote:
>
>> On 21/08/18 03:02, Timothy Arceri wrote:
>> > On 20/08/18 23:31, vadym.shovkoplias wrote:
>> >>  From Section 4.3.4 (Inputs) of the GLSL 1.50 spec:
>> >>
>> >>      "Only the input variables that are actually read need to be
>> written
>> >>       by the previous stage; it is allowed to have superfluous
>> >>       declarations of input variables."
>> >>
>> >> Fixes:
>> >>      * interstage-multiple-shader-objects.shader_test
>> >>
>> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101247
>> >> Signed-off-by: Vadym Shovkoplias <vadym.shovkoplias at globallogic.com>
>> >> ---
>> >>   src/compiler/glsl/link_interface_blocks.cpp | 8 +++++++-
>> >>   1 file changed, 7 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/src/compiler/glsl/link_interface_blocks.cpp
>> >> b/src/compiler/glsl/link_interface_blocks.cpp
>> >> index e5eca9460e..801fbcd5d9 100644
>> >> --- a/src/compiler/glsl/link_interface_blocks.cpp
>> >> +++ b/src/compiler/glsl/link_interface_blocks.cpp
>> >> @@ -417,9 +417,15 @@ validate_interstage_inout_blocks(struct
>> >> gl_shader_program *prog,
>> >>          * write to any of the pre-defined outputs (e.g. if the
>> >> vertex shader
>> >>          * does not write to gl_Position, etc), which is allowed and
>> >> results in
>> >>          * undefined behavior.
>> >> +       *
>> >> +       * From Section 4.3.4 (Inputs) of the GLSL 1.50 spec:
>> >> +       *
>> >> +       *    "Only the input variables that are actually read need to
>> >> be written
>> >> +       *     by the previous stage; it is allowed to have superfluous
>> >> +       *     declarations of input variables."
>> >>          */
>> >>         if (producer_def == NULL &&
>> >> -          !is_builtin_gl_in_block(var, consumer->Stage)) {
>> >> +          !is_builtin_gl_in_block(var, consumer->Stage) &&
>> >> var->data.used) {
>> >
>> > This concerns me a little. As far as I remember 'used' was added to
>> > make compiler warning better but it's not 100% reliable.
>>
>> +1 on the concerns thing. In addition to be used mostly for warnings, we
>> need to take into account his description comment at ir.h:
>>
>>      "
>>        * This is only maintained in the ast_to_hir.cpp path, not in
>>        * Mesa's fixed function or ARB program paths.
>>      "
>>
>> So if we start to use this while linking, then we need to ensure that it
>> is maintained outside the ast_to_hir pass (or at least, ensure that it
>> is correct after that pass). And if we got that, then that comment
>> became obsolete, and should be removed as part of the patch.
>>
>> >
>> >>            linker_error(prog, "Input block `%s' is not an output of "
>> >>                         "the previous stage\n",
>> >> var->get_interface_type()->name);
>> >>            return;
>> >>
>> > _______________________________________________
>> > mesa-dev mailing list
>> > mesa-dev at lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>>
>
>
> --
>
> Vadym Shovkoplias | Senior Software Engineer
> GlobalLogic
> P +380.57.766.7667  M +3.8050.931.7304  S vadym.shovkoplias
> www.globallogic.com
> <http://www.globallogic.com/>
> http://www.globallogic.com/email_disclaimer.txt
>
>
>


-- 

Vadym Shovkoplias | Senior Software Engineer
GlobalLogic
P +380.57.766.7667  M +3.8050.931.7304  S vadym.shovkoplias
www.globallogic.com
<http://www.globallogic.com/>
http://www.globallogic.com/email_disclaimer.txt
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180822/c02855f9/attachment.html>