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 <sigles...@igalia.com
<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 <tarc...@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
<apinhe...@igalia.com <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
<https://bugs.freedesktop.org/show_bug.cgi?id=101247>
>> Signed-off-by: Vadym Shovkoplias
<vadym.shovkopl...@globallogic.com
<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 list
> mesa-dev@lists.freedesktop.org
<mailto:mesa-dev@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
