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

[Timothy Arceri]

On 21/08/18 19: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 ?

I don't think it is but I'm willing to put this in the who cares basket. 
Worst case scenario we get an error message when we probably shouldn't.

I believe the spec text is worded this way so these unused blockes can 
be removed by opts during linking before validation is done. Ideally 
that is what we would do too. For now this patch is:

Reviewed-by: Timothy Arceri <tarceri at itsqueeze.com>

However I don't think you should update the comment Alejandro is taking 
about because I believe it is still correct. used is not set for fixed 
function or ARB asm style programs.

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