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

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

2018-08-28 Thread Vadym Shovkoplias

Hi Andreas,

Similar patch for varyings linking was pushed 4 years ago, so I think this
patch should be also stable.


On Tue, Aug 28, 2018 at 12:20 AM, Andres Gomez  wrote:

> Vadym, should we also include this in the stable queues ?
>
>
> On Mon, 2018-08-20 at 16:31 +0300, 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 
> > ---
> >  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) {
> >   linker_error(prog, "Input block `%s' is not an output of "
> >"the previous stage\n",
> var->get_interface_type()->name);
> >   return;
> --
> Br,
>
> Andres
>



-- 

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/email_disclaimer.txt
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2018-08-27 Thread Andres Gomez

Vadym, should we also include this in the stable queues ?


On Mon, 2018-08-20 at 16:31 +0300, 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 
> ---
>  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) {
>   linker_error(prog, "Input block `%s' is not an output of "
>"the previous stage\n", 
> var->get_interface_type()->name);
>   return;
-- 
Br,

Andres
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2018-08-23 Thread Alejandro Piñeiro

On 23/08/18 01:51, Timothy Arceri wrote:
> 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 >     >
>>     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 
>
> 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.

Sorry I didn't explain myself properly. Yes I agree that it is not used
for fixed functions or ARB programs, so that part would remain. IMHO,
the sentence that needs some tweaking is "This is only maintained in the
ast_to_hir.cpp path", as it seems to suggest that it is only
updated/valid during that pass, and now we are using it later, during
linking. Unless Im over-reading.

>
>>
>> On Tue, Aug 21, 2018 at 12:00 PM, Alejandro Piñeiro
>> mailto:apinhe...@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
>>     >     >
>>  >> ---
>>  >>   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:
>>
>>   "
>>

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

2018-08-22 Thread 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 mailto:sigles...@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 

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 
mailto:apinhe...@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
mailto:vadym.shovkopl...@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

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

2018-08-22 Thread 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 
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 
> 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 
> 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 
>> >> ---
>> >>   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@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/email_disclaimer.txt
>
>
>


-- 

Vadym Shovkoplias | Senior Software Engineer
GlobalLogic
P +380.57.766.7667  M

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

2018-08-22 Thread 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  >
> 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
> mailto:apinhe...@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
>  >
> >> ---
> >>   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@lists.freedesktop.org
> 
> >

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

2018-08-21 Thread Vadym Shovkoplias

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

On Tue, Aug 21, 2018 at 12:00 PM, Alejandro Piñeiro 
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 
> >> ---
> >>   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@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/email_disclaimer.txt
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2018-08-21 Thread Alejandro Piñeiro

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 
>> ---
>>   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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2018-08-20 Thread Timothy Arceri


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



   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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2018-08-20 Thread Marek Olšák

Reviewed-by: Marek Olšák 

Marek
On Mon, Aug 20, 2018 at 9:32 AM 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 
> ---
>  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) {
>   linker_error(prog, "Input block `%s' is not an output of "
>"the previous stage\n", 
> var->get_interface_type()->name);
>   return;
> --
> 2.18.0
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2018-08-20 Thread vadym.shovkoplias

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 
---
 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) {
  linker_error(prog, "Input block `%s' is not an output of "
   "the previous stage\n", var->get_interface_type()->name);
  return;
-- 
2.18.0

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

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

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

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

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

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

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

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

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

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

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

11 matches

Site Navigation

Mail list logo

Footer information