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

[Alejandro Piñeiro]

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
>     <mailto: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 <mailto: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
>     <https://bugs.freedesktop.org/show_bug.cgi?id=101247>
>     >> Signed-off-by: Vadym Shovkoplias
>     <vadym.shovkoplias at globallogic.com
>     <mailto: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
>     <mailto:mesa-dev at lists.freedesktop.org>
>     > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>     <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/>
> http://www.globallogic.com/email_disclaimer.txt

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180822/62f575a1/attachment-0001.html>