On Thursday 26 March 2015 23:10:53 Ben Widawsky wrote:
> On Mon, Dec 01, 2014 at 02:04:50PM +0100, Eduardo Lima Mitev wrote:
> > From: Samuel Iglesias Gonsalvez <sigles...@igalia.com>
> >
> > GLSL ES 3.00 spec, 4.3.10 (Linking of Vertex Outputs and Fragment Inputs),
> > page 45 says the following:
> >
> > "The type of vertex outputs and fragment input with the same name must
> > match, otherwise the link command will fail. The precision does not need
> > to match. Only those fragment inputs statically used (i.e. read) in the
> > fragment shader must be declared as outputs in the vertex shader;
> > declaring superfluous vertex shader outputs is permissible."
> > [...]
> > "The term static use means that after preprocessing the shader includes at
> > least one statement that accesses the input or output, even if that
> > statement is never actually executed."
> >
> > And it includes a table with all the possibilities.
> >
> > Similar table or content is present in other GLSL specs: GLSL 4.40, GLSL
> > 1.50, etc but for more stages (vertex and geometry shaders, etc).
> >
> > This patch detects that case and returns a link error. It fixes the
> > following>
> > dEQP test:
> > dEQP-GLES3.functional.shaders.linkage.varying.rules.illegal_usage_1
> >
> > However, it adds a new regression in piglit because the test hasn't a
> > vertex shader and it checks the link status.
> >
> > bin/glslparsertest \
> > tests/spec/glsl-1.50/compiler/gs-also-uses-smooth-flat-noperspective.geom
> > pass \ 1.50 --check-link
> >
> > This piglit test is wrong according to the spec wording above, so if this
> > patch is merged it should be updated.
> >
> > Signed-off-by: Samuel Iglesias Gonsalvez <sigles...@igalia.com>
> > ---
> >
> > src/glsl/link_varyings.cpp | 16 ++++++++++++++++
> > 1 file changed, 16 insertions(+)
> >
> > diff --git a/src/glsl/link_varyings.cpp b/src/glsl/link_varyings.cpp
> > index 1fe198a..036996f 100644
> > --- a/src/glsl/link_varyings.cpp
> > +++ b/src/glsl/link_varyings.cpp
> > @@ -263,6 +263,22 @@ cross_validate_outputs_to_inputs(struct
> > gl_shader_program *prog,>
> > if (output != NULL) {
> >
> > cross_validate_types_and_qualifiers(prog, input, output,
> >
> > consumer->Stage,
> > producer->Stage);
> >
> > + } else {
> > + /* Check for input vars with unmatched output vars in prev
> > stage + * taking into account that interface blocks could
> > have a match + * output but with different name, so we ignore
> > them. + */
> > + if (input->data.used && !input->data.assigned &&
> > + !(input->is_interface_instance() ||
> > + input->get_interface_type() ||
> > + input->is_in_uniform_block()) &&
> > + input->data.how_declared == ir_var_declared_normally &&
> > + input->data.location == -1)
> > + 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);
>
> The current code isn't wrong, but it made things somewhat confusing for me
> when reviewing. I don't think there is ever a case where
> input->data.assigned is valid. Inputs are supposed to be read only, and so
> they would never be assigned. How about instead:
> assert(!input->data.assigned);
> OK, I will do it. > I am also not certain if how_declared is correct. It seems like we shouldn't > get this far if that wasn't true, but I am not an expert (and I can't see > it being wrong). > I am going to check that part again, just in case how_declared is not needed at this point. > With the data.assigned change, it's > Reviewed-by: Ben Widawsky <b...@bwidawsk.net> > Thanks for your review! Sam > > } > > > > } > > > > }
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
