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@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
