[Mesa-dev] [PATCH v2] mesa: fix interface matching done in validate_io
Tapani Pälli
tapani.palli at intel.com
Tue Dec 15 02:51:21 PST 2015
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?
>>>> ---
>>>> 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
More information about the mesa-dev
mailing list