[Mesa-dev] [PATCH] glsl/linker: Allow unused in blocks which are not declated on previous stage
Alejandro Piñeiro
apinheiro at igalia.com
Thu Aug 23 09:03:24 UTC 2018
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 <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.
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
>> <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
>
More information about the mesa-dev
mailing list