[Mesa-dev] [PATCH v2] mesa: fix interface matching done in validate_io
Timothy Arceri
timothy.arceri at collabora.com
Tue Dec 15 03:25:52 PST 2015
On Tue, 2015-12-15 at 12:51 +0200, Tapani Pälli wrote:
> On 12/15/2015 10:56 AM, Timothy Arceri wrote:
> > On Tue, 2015-12-15 at 07:58 +0200, Tapani Pälli wrote:
> > > On 12/15/2015 03:31 AM, Timothy Arceri wrote:
> > > > On Mon, 2015-12-14 at 10:29 +0200, Tapani Pälli wrote:
> > > > > Patch makes following changes for interface matching:
> > > > >
> > > > > - do not try to match builtin variables
> > > > > - handle swizzle in input name, as example 'a.z' should
> > > > > match with 'a'
> > > > > - check that amount of inputs and outputs matches
> > > > >
> > > > > These changes make interface matching tests to work in:
> > > > > ES31-CTS.sepshaderobjs.StateInteraction
> > > > >
> > > > > Test does not still pass completely due to errors in
> > > > > rendering
> > > > > output. IMO this is unrelated to interface matching.
> > > > >
> > > > > v2: add spec reference, return true on desktop since we do
> > > > > not
> > > > > have failing cases for it, inputs and outputs amount do
> > > > > not
> > > > > need to match on desktop.
> > > > >
> > > > > Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
> > > > Hi Tapani,
> > > >
> > > > Just a general comment first.
> > > >
> > > > I think we should first move _mesa_validate_pipeline_io() and
> > > > validate_io() to src/mesa/main/pipelineobj.c I don't think
> > > > it
> > > > belongs
> > > > here right?
> > > Sure, it can be done now. Original intention was to use program
> > > resources and that is why it ended up being here.
>
> Ah but it uses ir_variable so it may be painful to move. Would it be
> OK
> to still have it in shader_query.cpp?
If its not straight forword to move then thats fine for now. I'd rather
get this fixed then worry about where the code it located :)
>
> > > > > ---
> > > > > src/mesa/main/shader_query.cpp | 54
> > > > > ++++++++++++++++++++++++++++++++++++++----
> > > > > 1 file changed, 50 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/src/mesa/main/shader_query.cpp
> > > > > b/src/mesa/main/shader_query.cpp
> > > > > index ced10a9..bc01b97 100644
> > > > > --- a/src/mesa/main/shader_query.cpp
> > > > > +++ b/src/mesa/main/shader_query.cpp
> > > > > @@ -1377,19 +1377,38 @@ validate_io(const struct gl_shader
> > > > > *input_stage,
> > > > > const struct gl_shader *output_stage, bool
> > > > > isES)
> > > > > {
> > > > > assert(input_stage && output_stage);
> > > > > + unsigned inputs = 0, outputs = 0;
> > > > > +
> > > > > + /* Currently no matching done for desktop. */
> > > > I think the spec quote should be moved here as it applies to
> > > > all
> > > > the
> > > > rules in the function then you can also have the comment
> > > > explaining
> > > > why
> > > > validation for desktop it not done.
> > > OK
> > >
> > > > I've also filed a spec bug for desktop for the reasons I
> > > > outlined
> > > > in
> > > > irc previously. It would be great if you could quote the bug
> > > > here
> > > > also.
> > > > Something like:
> > > >
> > > > /* FIXME: Update once Khronos spec bug #15331 is resolved. */
> > > Sure, will add.
> > >
> > > > > + if (!isES)
> > > > > + return true;
> > > > >
> > > > > /* For each output in a, find input in b and do any
> > > > > required
> > > > > checks. */
> > > > > foreach_in_list(ir_instruction, out, input_stage->ir) {
> > > > > ir_variable *out_var = out->as_variable();
> > > > It's existing code but it would also be nice to have a patch
> > > > that
> > > > renames input_stage/output_stage to
> > > > producer_stage/consumer_stage
> > > > this
> > > > it what they are called in the linker code. Maybe its just me
> > > > but
> > > > getting the outputs from input_stage just looks wrong.
> > > OK, can change this.
> > >
> > > > > - if (!out_var || out_var->data.mode !=
> > > > > ir_var_shader_out)
> > > > > + if (!out_var || out_var->data.mode !=
> > > > > ir_var_shader_out ||
> > > > > + is_gl_identifier(out_var->name))
> > > > > continue;
> > > > >
> > > > > + outputs++;
> > > > > +
> > > > > + inputs = 0;
> > > > > foreach_in_list(ir_instruction, in, output_stage
> > > > > ->ir) {
> > > > Two comments here:
> > > >
> > > > 1. Take a look at cross_validate_outputs_to_inputs() in
> > > > link_varyings.cpp for a way to avoid the nested loop? Although
> > > > it
> > > > may
> > > > cause even more overhaed using the symbol table not sure.
> > > I don't know if symbol table can be trusted as variables that get
> > > optimized away or changed in some way are still there. Only way
> > > to be
> > > sure is to iterate IR or use resource list. Also, symbol table
> > > gets
> > > destroyed after linking. My first implementation was using a hash
> > > but
> > > that was also bad idea because variables names do not necessarily
> > > match
> > > exactly.
> >
> > The code in in cross_validate_outputs_to_inputs() doesn't use *the*
> > symbol table it use *a* which it builds from iterating over the
> > producers IR. But I guess it will have the same problem as the hash
> > table. I wonder why the linking code uses it rather than a plain
> > hash
> > table.
> >
> >
> > > > 2. Take a look at the same function for matching via explicit
> > > > location.
> > > > Does the CTS not test for mismatched explicit locations? Maybe
> > > > we
> > > > should create a piglit test for this as your existing code
> > > > doesn't
> > > > take
> > > > into account explicit locations.
> > > No, I haven't seen this test using explicit locations. This patch
> > > also
> > > makes the interface matching pass.
> > Right but it would break any varyings with explicit locations that
> > don't have a matching names which is legal.
> >
> > "An output variable is considered to match an input variable in the
> > subsequent shader if:
> >
> > –the two variables match in name, type, and qualification; or
> > –the two variables are declared with the same location qualifier
> > and
> > match in type and qualification."
> >
> >
> > > > I was going to suggest sharing the code between here and the
> > > > linker
> > > > however I'm about to add a bunch of rules for matching the
> > > > component
> > > > qualifier for enhanced layouts so not entirely sure if we
> > > > should do
> > > > this what do you think?
> > > Linker will need to do much more so maybe do separately, at least
> > > for
> > > now?
> > Yeah I think that's a good idea for now.
> >
> >
> > > > > ir_variable *in_var = in->as_variable();
> > > > > - if (!in_var || in_var->data.mode !=
> > > > > ir_var_shader_in)
> > > > > + if (!in_var || in_var->data.mode !=
> > > > > ir_var_shader_in ||
> > > > > + is_gl_identifier(in_var->name))
> > > > > continue;
> > > > >
> > > > > - if (strcmp(in_var->name, out_var->name) == 0) {
> > > > > + inputs++;
> > > > > +
> > > > > + unsigned len = strlen(in_var->name);
> > > > > +
> > > > > + /* Handle input swizzle in variable name. */
> > > > > + const char *dot = strchr(in_var->name, '.');
> > > > > + if (dot)
> > > > > + len = dot - in_var->name;
> > > > Hmm ... I wonder if this is also missing from the linker or
> > > > maybe
> > > > the
> > > > symbol table stuff handles this.
> > > Variable names get mangled during optimizations so symbol table
> > > should
> > > have the correct names left during linking.
> > > > > +
> > > > > + if (strncmp(in_var->name, out_var->name, len) == 0)
> > > > > {
> > > > > /* Since we now only validate precision, we
> > > > > can
> > > > > skip
> > > > > this step for
> > > > > * desktop GLSL shaders, there precision
> > > > > qualifier
> > > > > is
> > > > > ignored.
> > > > > *
> > > > > @@ -1412,7 +1431,34 @@ validate_io(const struct gl_shader
> > > > > *input_stage,
> > > > > }
> > > > > }
> > > > > }
> > > > > - return true;
> > > > > +
> > > > > + /* Amount of inputs vs outputs should match when using
> > > > > OpenGL
> > > > > ES.
> > > > > + *
> > > > > + * From OpenGL ES 3.1 spec (Interface matching):
> > > > > + *
> > > > > + * "At an interface between program objects, the set
> > > > > of
> > > > > inputs
> > > > > and outputs
> > > > > + * are considered to match exactly if and only if:
> > > > > + *
> > > > > + * - Every declared input variable has a matching
> > > > > output,
> > > > > as
> > > > > described
> > > > > + * above.
> > > > > + *
> > > > > + * - There are no user-defined output variables
> > > > > declared
> > > > > without a
> > > > > + * matching input variable declaration.
> > > > > + *
> > > > > + * - All matched input and output variables have
> > > > > identical
> > > > > precision
> > > > > + * qualification.
> > > > > + *
> > > > > + * When the set of inputs and outputs on an interface
> > > > > between
> > > > > programs
> > > > > + * matches exactly, all inputs are well-defined except
> > > > > when
> > > > > the
> > > > > + * corresponding outputs were not written in the
> > > > > previous
> > > > > shader. However,
> > > > > + * any mismatch between inputs and outputs will result
> > > > > in
> > > > > a
> > > > > validation
> > > > > + * failure."
> > > > > + *
> > > > > + * OpenGL Core 4.5 spec includes same paragraph as above
> > > > > but
> > > > > without last
> > > > > + * precision or the last 'validation failure' clause.
> > > > > Therefore
> > > > > behaviour is
> > > > > + * more relaxed, input and output amount does not need to
> > > > > match
> > > > > on desktop.
> > > > Well they do need to match if they are all used but it doesn't
> > > > seem
> > > > the
> > > > spec requires it to validated so maybe "does not need to match"
> > > > ->
> > > > "is
> > > > not required by the spec to be validated".
> > > OK, will change.
> > >
> > > > You are also exiting for desktop at the top of the if below is
> > > > not
> > > > required.
> > > It is 'future-proofing' if/when we will have some desktop rules
> > > here
> > > as
> > > well. My assumption was that we will have some but looking at how
> > > Nvidia
> > > driver works, I don't think they have any rules at all for this
> > > so
> > > might
> > > be that it will not happen.
> > Yeah I think the answer to the bug I filed will be to remove the
> > wording about interface matching from the desktop spec. I found
> > another
> > old bug when searching to see it there was any existing bug that
> > was
> > talking about providing an API to query with so the validation can
> > be
> > done on the application side. In other words it seems to be an
> > applications job to make sure things are right.
> >
> > > > > + */
> > > > > + return isES ? inputs == outputs : true;
> > > > > }
> > > > >
> > > > > /**
> > > // Tapani
> > >
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev at lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list