On Wed, 2015-12-16 at 19:29 +1100, Timothy Arceri wrote: > On Wed, 2015-12-16 at 08:24 +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' > > - add matching by location > > - 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 > > Test does not still pass -> The test still does not pass > > > output. IMO this is unrelated to interface matching. > > > > Note that type matching is not done due to varying packing which > > changes type of variable, this can be added later on. Preferably > > when we have quicker way to iterate resources and have a complete > > list of all existed varyings (before packing) available. > > > > 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. > > > > v3: add some more spec reference, remove desktop specifics since > > not used for now on desktop, add match by location qualifier, > > rename input_stage and output_stage as producer and consumer > > as suggested by Timothy. > > > > Signed-off-by: Tapani Pälli <tapani.pa...@intel.com> > > --- > > src/mesa/main/shader_query.cpp | 110 > > +++++++++++++++++++++++++++++++ > > ---------- > > 1 file changed, 83 insertions(+), 27 deletions(-) > > > > diff --git a/src/mesa/main/shader_query.cpp > > b/src/mesa/main/shader_query.cpp > > index ced10a9..ea2b2f4 100644 > > --- a/src/mesa/main/shader_query.cpp > > +++ b/src/mesa/main/shader_query.cpp > > @@ -1373,46 +1373,102 @@ _mesa_get_program_resourceiv(struct > > gl_shader_program *shProg, > > } > > > > static bool > > -validate_io(const struct gl_shader *input_stage, > > - const struct gl_shader *output_stage, bool isES) > > +validate_io(const struct gl_shader *producer, > > + const struct gl_shader *consumer, bool isES) > > { > > - assert(input_stage && output_stage); > > + assert(producer && consumer); > > + unsigned inputs = 0, outputs = 0; > > + > > + /* From OpenGL ES 3.1 spec (Interface matching): > > + * > > + * "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. > > + * > > + * ... > > + * > > + * 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 check > > + * for precision and the last 'validation failure' clause. > > Therefore > > + * behaviour is more relaxed, input and output amount is not > > required by the > > + * spec to be validated. > > + * > > + * FIXME: Update once Khronos spec bug #15331 is resolved. > > + * FIXME: Add validation by type, currently information loss > > during varying > > + * packing makes this challenging. > > + */ > > + > > + /* Currently no matching done for desktop. */ > > + 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) { > > + foreach_in_list(ir_instruction, out, producer->ir) { > > ir_variable *out_var = out->as_variable(); > > - 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)) > > Another way to do this would be: > > out_var->data.location < VARYING_SLOT_VAR0 > > Rather than: > > is_gl_identifier(out_var->name) > > If you make this change don't worry about sending out a new revision. > > > continue; > > > > - foreach_in_list(ir_instruction, in, output_stage->ir) { > > + outputs++; > > + > > + inputs = 0; > > + foreach_in_list(ir_instruction, in, consumer->ir) { > > 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)) > > Same as above here. > > > > continue; > > > > - if (strcmp(in_var->name, out_var->name) == 0) { > > - /* Since we now only validate precision, we can skip > > this step for > > - * desktop GLSL shaders, there precision qualifier is > > ignored. > > - * > > - * From OpenGL 4.50 Shading Language spec, section > > 4.7: > > - * "For the purposes of determining if an output > > from one > > - * shader stage matches an input of the next > > stage, > > the > > - * precision qualifier need not match." > > + inputs++; > > + > > + /* Match by location qualifier and precision. */ > > + if ((in_var->data.explicit_location && > > + out_var->data.explicit_location) && > > + in_var->data.location == out_var->data.location && > > + in_var->data.precision == out_var->data.precision) > > + continue; > > The ES SL spec doesn't seem to say anything about what happens if one > stage has and explicit location and the other doesn't but falling > through and matching a varying with an explicit location by name > seems > like a bad idea. > > This seems like an oversight in the ES spec, maybe bug report worthy? > > For exaple you could end up with something like this being declared > valid. > > [vertex shader] > layout(location = 7) out vec3 a; > out vec3 b; > out vec3 c; > > [fragment shader] > out vec3 a; > layout(location = 5) out vec3 b; > out vec3 c; > > The desktop spec has this line: > > "For the purposes of determining if a non-vertex input matches an > output from a previous shader stage, the location layout qualifier > (if > any) must match." > > We fail linking if the explicit location does not have a matching > explicit location. > > For the sake of fixing the name matching tests, if you change the > above > to something like: > > /* FIXME: Add explicit location matching validation here. Be careful > not to match varyings with explicit locations to varyings without > explicit locations. */ > if (in_var->data.explicit_location) > continue;
In the commit you added the comment but left the old code: if ((in_var->data.explicit_location out_var->data.explicit_location) in_var->data.location == out_var->data.location in_var->data.precision == out_var->data.precision) continue; Rather than just skipping it for now: if (in_var->data.explicit_location) continue; > > If you file a bug then maybe add that to the comment too. > > And with my other minor comments addressed. > > Reviewed-by: Timothy Arceri <timothy.arc...@collabora.com> > > > + > > + 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; > > + > > + /* Match by name and precision. */ > > + if (strncmp(in_var->name, out_var->name, len) == 0) { > > + /* From OpenGL ES 3.1 spec: > > + * "When both shaders are in separate programs, > > mismatched > > + * precision qualifiers will result in a program > > interface > > + * mismatch that will result in program pipeline > > validation > > + * failures, as described in section 7.4.1 > > (“Shader > > Interface > > + * Matching”) of the OpenGL ES 3.1 Specification." > > */ > > - if (isES) { > > - /* From OpenGL ES 3.1 spec: > > - * "When both shaders are in separate programs, > > mismatched > > - * precision qualifiers will result in a > > program > > interface > > - * mismatch that will result in program > > pipeline > > validation > > - * failures, as described in section 7.4.1 > > (“Shader Interface > > - * Matching”) of the OpenGL ES 3.1 > > Specification." > > - */ > > - if (in_var->data.precision != out_var > > ->data.precision) > > - return false; > > - } > > + if (in_var->data.precision != out_var->data.precision) > > + return false; > > } > > } > > } > > - return true; > > + return inputs == outputs; > > } > > > > /** > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev