[Mesa-dev] [PATCH 3/3] glsl/linker: Check that re-declared, inter-shader built-in blocks match
Timothy Arceri
t_arceri at yahoo.com.au
Tue Jan 31 11:21:30 UTC 2017
On Tue, 2017-01-31 at 11:39 +0100, Eduardo Lima Mitev wrote:
> On 01/31/2017 11:15 AM, Timothy Arceri wrote:
> > On Tue, 2017-01-31 at 10:28 +0100, Eduardo Lima Mitev wrote:
> > > On 01/27/2017 11:32 PM, Timothy Arceri wrote:
> > > > On Fri, 2017-01-27 at 17:42 +0100, Eduardo Lima Mitev wrote:
> > > > > From GLSL 4.5 spec, section "7.1 Built-In Language
> > > > > Variables",
> > > > > page
> > > > > 130 of
> > > > > the PDF states:
> > > > >
> > > > > "If multiple shaders using members of a built-in block
> > > > > belonging
> > > > > to
> > > > > the same interface are linked together in the same
> > > > > program,
> > > > > they
> > > > > must
> > > > > all redeclare the built-in block in the same way, as
> > > > > described
> > > > > in
> > > > > section 4.3.9 “Interface Blocks” for interface-block
> > > > > matching,
> > > > > or a
> > > > > link-time error will result."
> > > > >
> > > > > Fixes:
> > > > > * GL45-CTS.CommonBugs.CommonBug_PerVertexValidation
> > > >
> > > >
> > > > I was looking at this test yesterday and noticed the the GS
> > > > input
> > > > is
> > > > not used, so technically there is no reason the builtin can't
> > > > be
> > > > optimised away. This means there is no need for validation
> > > > between
> > > > the
> > > > GS interface and whatever output it is linked against, and is
> > > > IMO a
> > > > bug
> > > > in the test.
> > > >
> > >
> > > This is a valid point and was also my first impression when I
> > > looked
> > > into the test. My argument to go and fix this in Mesa is that the
> > > fact
> > > that a driver optimizes out an unused variable is/should be
> > > transparent
> > > from a spec point of view. A different driver could very well
> > > fail
> > > linkage and would not be violating the spec either.
> >
> > Usually implementations will only throw errors when a spec says to
> > do
> > so, unless there is a technical reason we need to throw one. Being
> > restrictive when the spec doesn't say to usually leads to trouble
> > as
> > something will work on one implementation and not on another. If
> > there
> > is no technical reason not to allow something you don't want to be
> > the
> > implementation that's failing to load the app.
> >
> > If you really think it should be disallowed then I think you should
> > file a spec bug.
> >
>
> Ok, fair enough.
>
> >
> > >
> > > > Therefore I'm concerned that this series is forcing validation
> > > > on
> > > > unused varyings which isn't required by the spec.
> > > >
> > >
> > > This is true. But the way I see it, there is a stricter and a
> > > relaxed
> > > way of looking at this. Your concern above materializes when a
> > > shader
> > > has a built-in block re-declared, it is incompatible with
> > > previous/next
> > > shader's interface, and the variable is unused. Failing linkage
> > > in
> > > this
> > > scenario is certainly not in the spec, but perhaps the most
> > > sensible
> > > thing to so. The shaders are wrong. If I was a shader developer,
> > > I
> > > think
> > > I would prefer linkage to fail here, just in case I have a legit
> > > bug
> > > in
> > > my shaders.
> > >
> > > In any case, I already have a local fix for the test, which I
> > > plan
> > > submit.
> >
> > Did the fix cause the test to pass without your series? When I
> > tried it
> > still failed but maybe I did something wrong.
> >
>
> Yes, making the unused variable used, test passes.
>
> For example, in
> https://github.com/KhronosGroup/VK-GL-CTS/blob/master/external/opengl
> cts/modules/gl/gl3cCommonBugsTests.cpp#L2270:
>
> Change:
>
> gl_ClipDistance[0] = 0.5;
>
> by:
>
> gl_ClipDistance[0] = gl_in[0].gl_ClipDistance[0] + 0.1;
Nice :) Not sure what I did wrong. Maybe I was testing with a buggy
Mesa I was playing with at the same time.
>
> > > So I'm open to drop the series if we still think we should not
> > > be this strict.
> >
> > As I said above I'd rather we not be more strict that the spec asks
> > for, if you really want to change it then please file a spec bug.
> >
>
> Ok. I will drop the series, and only submit the fix for the code
> duplication.
Cool. I'll try take a look over those.
>
> Eduardo
>
> > > However, code gets factorized in patches 1 and 2
> > > regardless, so will send a v2 for those (dropping the copying of
> > > blocks).
> > >
> > > Thanks for the feedback, Timothy!
> > >
> > > Eduardo
> > >
> > > >
> > > > > ---
> > > > > src/compiler/glsl/link_interface_blocks.cpp | 33
> > > > > ++++++++++++++++++++++++++++-
> > > > > 1 file changed, 32 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/src/compiler/glsl/link_interface_blocks.cpp
> > > > > b/src/compiler/glsl/link_interface_blocks.cpp
> > > > > index 7037c7776de..4c0278661fa 100644
> > > > > --- a/src/compiler/glsl/link_interface_blocks.cpp
> > > > > +++ b/src/compiler/glsl/link_interface_blocks.cpp
> > > > > @@ -376,11 +376,42 @@ validate_interstage_inout_blocks(struct
> > > > > gl_shader_program *prog,
> > > > > /* Verify that the consumer's input interfaces match. */
> > > > > foreach_in_list(ir_instruction, node, consumer->ir) {
> > > > > ir_variable *var = node->as_variable();
> > > > > - if (!var || !var->get_interface_type() || var-
> > > > > >data.mode
> > > > > !=
> > > > > ir_var_shader_in)
> > > > > + if (!var || !var->get_interface_type())
> > > > > continue;
> > > > >
> > > > > ir_variable *producer_def = definitions.lookup(var);
> > > > >
> > > > > + /* Check that all built-in block re-declarations are
> > > > > compatible
> > > > > + * across shaders: From OpenGL Shading Language 4.5,
> > > > > section
> > > > > + * "7.1 Built-In Language Variables", page 130 of the
> > > > > PDF:
> > > > > + *
> > > > > + * "If multiple shaders using members of a built-in
> > > > > block
> > > > > belonging
> > > > > + * to the same interface are linked together in
> > > > > the
> > > > > same
> > > > > program,
> > > > > + * they must all redeclare the built-in block in
> > > > > the
> > > > > same
> > > > > way, as
> > > > > + * described in section 4.3.9 “Interface Blocks”
> > > > > for
> > > > > interface-block
> > > > > + * matching, or a link-time error will result."
> > > > > + */
> > > > > + const glsl_type *consumer_iface =
> > > > > + consumer->symbols->get_interface(var-
> > > > > > get_interface_type()-
> > > > > > name,
> > > > >
> > > > > + ir_var_shader_in);
> > > > > +
> > > > > + const glsl_type *producer_iface = NULL;
> > > > > + if (producer_def && producer_def-
> > > > > >get_interface_type()) {
> > > > > + producer_iface =
> > > > > + producer->symbols->get_interface(producer_def-
> > > > > > get_interface_type()->name,
> > > > >
> > > > > + ir_var_shader_o
> > > > > ut);
> > > > > + }
> > > > > +
> > > > > + if (producer_iface && consumer_iface &&
> > > > > + interstage_member_mismatch(prog, consumer_iface,
> > > > > producer_iface)) {
> > > > > + linker_error(prog, "Incompatible or missing
> > > > > gl_PerVertex
> > > > > re-declaration"
> > > > > + "in consecutive shaders");
> > > > > + return;
> > > > > + }
> > > > > +
> > > > > + if (var->data.mode != ir_var_shader_in)
> > > > > + continue;
> > > > > +
> > > > > /* The producer doesn't generate this input: fail to
> > > > > link.
> > > > > Skip built-in
> > > > > * 'gl_in[]' since that may not be present if the
> > > > > producer
> > > > > does not
> > > > > * write to any of the pre-defined outputs (e.g. if
> > > > > the
> > > > > vertex
> > > > > shader
> > >
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list