[Mesa-dev] [PATCH 3/3] glsl/linker: Check that re-declared, inter-shader built-in blocks match
Eduardo Lima Mitev
elima at igalia.com
Tue Jan 31 10:39:27 UTC 2017
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/openglcts/modules/gl/gl3cCommonBugsTests.cpp#L2270:
Change:
gl_ClipDistance[0] = 0.5;
by:
gl_ClipDistance[0] = gl_in[0].gl_ClipDistance[0] + 0.1;
>> 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.
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_out);
>>>> + }
>>>> +
>>>> + 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
>
More information about the mesa-dev
mailing list