[Mesa-dev] [PATCH V2 3/4] glsl: Link error if fs defines conflicting qualifiers for gl_FragCoord
Anuj Phogat
anuj.phogat at gmail.com
Tue Feb 25 17:02:10 PST 2014
On Tue, Feb 25, 2014 at 9:14 AM, Ian Romanick <idr at freedesktop.org> wrote:
> On 02/24/2014 05:34 PM, Anuj Phogat wrote:
>> GLSL 1.50 spec says:
>> "If gl_FragCoord is redeclared in any fragment shader in a program,
>> it must be redeclared in all the fragment shaders in that
>> program that have a static use gl_FragCoord. All redeclarations of
>> gl_FragCoord in all fragment shaders in a single program must
>> have the same set of qualifiers."
>>
>> This patch causes the shader link to fail if we have multiple fragment
>> shaders with conflicting layout qualifiers for gl_FragCoord. For example:
>>
>> fragment shader 1:
>> layout(origin_upper_left, pixel_center_integer) in vec4 gl_FragCoord;
>>
>> void foo();
>> void main()
>> {
>> gl_FragColor = vec4(gl_FragCoord.xyz, 1.0);
>> foo();
>> }
>>
>> fragment shader 2:
>> layout(origin_upper_left) in vec4 gl_FragCoord;
>> void foo()
>> {
>> gl_FragColor.a = gl_FragCoord.z;
>> }
>>
>> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
>> Cc: <mesa-stable at lists.freedesktop.org>
>> ---
>> src/glsl/ast_to_hir.cpp | 5 ++++
>> src/glsl/glsl_parser_extras.cpp | 16 +++++++++++
>> src/glsl/glsl_parser_extras.h | 1 +
>> src/glsl/linker.cpp | 60 +++++++++++++++++++++++++++++++++++++++++
>> src/mesa/main/mtypes.h | 8 ++++++
>> 5 files changed, 90 insertions(+)
>>
>> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
>> index f5dacfd..b639f98 100644
>> --- a/src/glsl/ast_to_hir.cpp
>> +++ b/src/glsl/ast_to_hir.cpp
>> @@ -120,6 +120,11 @@ _mesa_ast_to_hir(exec_list *instructions, struct _mesa_glsl_parse_state *state)
>> instructions->push_head(var);
>> }
>>
>> + /* Figure out if gl_FragCoord is actually used in fragment shader */
>> + ir_variable *const var = state->symbols->get_variable("gl_FragCoord");
>> + if (var != NULL)
>> + state->fs_uses_gl_fragcoord = var->data.used;
>> +
>> /* From section 7.1 (Built-In Language Variables) of the GLSL 4.10 spec:
>> *
>> * If multiple shaders using members of a built-in block belonging to
>> diff --git a/src/glsl/glsl_parser_extras.cpp b/src/glsl/glsl_parser_extras.cpp
>> index fcbbf44..f953713 100644
>> --- a/src/glsl/glsl_parser_extras.cpp
>> +++ b/src/glsl/glsl_parser_extras.cpp
>> @@ -201,6 +201,7 @@ _mesa_glsl_parse_state::_mesa_glsl_parse_state(struct gl_context *_ctx,
>> this->default_uniform_qualifier->flags.q.shared = 1;
>> this->default_uniform_qualifier->flags.q.column_major = 1;
>>
>> + this->fs_uses_gl_fragcoord = false;
>> this->fs_redeclares_gl_fragcoord = false;
>> this->fs_origin_upper_left = false;
>> this->fs_pixel_center_integer = false;
>> @@ -1363,6 +1364,14 @@ set_shader_inout_layout(struct gl_shader *shader,
>> assert(!state->cs_input_local_size_specified);
>> }
>>
>> + if (shader->Stage != MESA_SHADER_FRAGMENT) {
>> + /* Should have been prevented by the parser. */
>> + assert(!state->fs_uses_gl_fragcoord);
>> + assert(!state->fs_redeclares_gl_fragcoord);
>> + assert(!state->fs_pixel_center_integer);
>> + assert(!state->fs_origin_upper_left);
>> + }
>> +
>> switch (shader->Stage) {
>> case MESA_SHADER_GEOMETRY:
>> shader->Geom.VerticesOut = 0;
>> @@ -1396,6 +1405,13 @@ set_shader_inout_layout(struct gl_shader *shader,
>> }
>> break;
>>
>> + case MESA_SHADER_FRAGMENT:
>> + shader->redeclares_gl_fragcoord = state->fs_redeclares_gl_fragcoord;
>> + shader->uses_gl_fragcoord = state->fs_uses_gl_fragcoord;
>> + shader->pixel_center_integer = state->fs_pixel_center_integer;
>> + shader->origin_upper_left = state->fs_origin_upper_left;
>> + break;
>> +
>> default:
>> /* Nothing to do. */
>> break;
>> diff --git a/src/glsl/glsl_parser_extras.h b/src/glsl/glsl_parser_extras.h
>> index 2017913..511ca48 100644
>> --- a/src/glsl/glsl_parser_extras.h
>> +++ b/src/glsl/glsl_parser_extras.h
>> @@ -427,6 +427,7 @@ struct _mesa_glsl_parse_state {
>> const struct gl_extensions *extensions;
>>
>> bool uses_builtin_functions;
>> + bool fs_uses_gl_fragcoord;
>>
>> /**
>> * For geometry shaders, size of the most recently seen input declaration
>> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
>> index f6b2661..1268aef 100644
>> --- a/src/glsl/linker.cpp
>> +++ b/src/glsl/linker.cpp
>> @@ -1194,6 +1194,65 @@ private:
>> hash_table *unnamed_interfaces;
>> };
>>
>> +static bool
>> +fs_uses_conflicting_layout_qualifiers(struct gl_shader *shader,
>> + struct gl_shader *linked_shader)
>> +{
>> + bool qual_absent_in_shader_but_present_in_linked_shader =
>> + (!shader->origin_upper_left && linked_shader->origin_upper_left) ||
>> + (!shader->pixel_center_integer && linked_shader->pixel_center_integer);
>> +
>> + bool qual_present_in_shader_but_absent_in_linked_shader =
>> + (shader->origin_upper_left && !linked_shader->origin_upper_left) ||
>> + (shader->pixel_center_integer && !linked_shader->pixel_center_integer);
>> +
>> + return ((qual_absent_in_shader_but_present_in_linked_shader &&
>> + shader->uses_gl_fragcoord) ||
>> + (qual_present_in_shader_but_absent_in_linked_shader &&
>> + linked_shader->uses_gl_fragcoord));
>
> I don't think this logic catches the redefined-but-not-used cases.
>
> layout(origin_upper_left) in vec4 gl_FragCoord;
> void main()
> {
> foo();
> gl_FragColor = gl_FragData;
> }
>
> ---
>
> layout(pixel_center_integer) in vec4 gl_FragCoord;
> void foo()
> {
> }
>
Yes, I missed this case :(. I'll add a piglit shader test for this.
> I think in the loop in link_fs_input_layout_qualifiers you want:
>
> /* From the GLSL 1.50 spec, page 39:
> *
> * "If gl_FragCoord is redeclared in any fragment shader in a program,
> * it must be redeclared in all the fragment shaders in that program
> * that have a static use gl_FragCoord.
> */
> if ((linked_shader->redeclares_gl_fragcoord
> && !shader->redeclares_gl_fragcoord
> && shader->uses_gl_fragcoord)
> || (shader->redeclares_gl_fragcoord
> && !linked_shader->redeclares_gl_fragcoord
> && linked_shader->uses_gl_fragcoord)) {
> // error
As discussed earlier, we need an exception for following case:
in vec4 gl_FragCoord;
void main()
{
foo();
gl_FragColor = gl_FragData;
}
---
void foo()
{
gl_FragColor.a = gl_FragCoord.z;
}
Using another check before error takes care of this:
/* Exclude the case when linked_shader redeclares gl_FragCoord
* with no layout qualifiers. GLSL 1.50 spec doesn't say anything
* specific about this case. Generating link error will be a wrong
* behavior and this could also break few applications.
* */
if (linked_shader->origin_upper_left
|| linked_shader->pixel_center_integer) {
// error
}
> }
>
> /* From the GLSL 1.50 spec, page 39:
> *
> * "All redeclarations of gl_FragCoord in all fragment shaders in a
> * single program must have the same set of qualifiers."
> */
> if (linked_shader->redeclares_gl_fragcoord && shader->redeclares_gl_fragcoord
> && (shader->origin_upper_left != linked_shader->origin_upper_left
> || shader->pixel_center_integer != linked_shader->pixel_center_integer)){
> // error
> }
>
> /* Update the linked shader state. Note that uses_gl_fragcoord should
> * accumulate the results. The other values should replace. If there
> * are multiple redeclarations, all the fields except uses_gl_fragcoord
> * are already known to be the same.
> */
> if (shader->redeclares_gl_fragcoord) {
linked_shader->uses_gl_fragcoord stays false while linking second fragment
shader in following example and both shaders link successfully:
[fragment shader]
void alpha();
void blue();
void main()
{
gl_FragColor = vec4(gl_FragCoord.xy, 0.0, 1.0);
alpha();
}
[fragment shader]
layout(origin_upper_left, pixel_center_integer) in vec4 gl_FragCoord;
void alpha()
{
gl_FragColor.a = gl_FragCoord.z;
}
So, we need to change the if condition to:
if (shader->redeclares_gl_fragcoord || shader->uses_gl_fragcoord)
> linked_shader->uses_gl_fragcoord = linked_shader->uses_gl_fragcoord
> || shader->uses_gl_fragcoord;
> linked_shader->origin_upper_left = shader->origin_upper_left;
> linked_shader->pixel_center_integer = shader->pixel_center_integer;
Also need:
linked_shader->redeclares_gl_fragcoord = true;
After making above changes to the suggested code, it seems to take
care of all the cases.
> }
>
>> +}
>> +
>> +/**
>> + * Performs the cross-validation of layout qualifiers specified in
>> + * redeclaration of gl_FragCoord for the attached fragment shaders,
>> + * and propagates them to the linked FS and linked shader program.
>> + */
>> +static void
>> +link_fs_input_layout_qualifiers(struct gl_shader_program *prog,
>> + struct gl_shader *linked_shader,
>> + struct gl_shader **shader_list,
>> + unsigned num_shaders)
>> +{
>> + linked_shader->uses_gl_fragcoord = false;
>> + linked_shader->origin_upper_left = false;
>> + linked_shader->pixel_center_integer = false;
inked_shader->redeclares_gl_fragcoord = false;
>> +
>> + if (linked_shader->Stage != MESA_SHADER_FRAGMENT || prog->Version < 150)
>> + return;
>> +
>> + /* From the GLSL 1.50 spec, page 39:
>> + * "If gl_FragCoord is redeclared in any fragment shader in a program,
>> + * it must be redeclared in all the fragment shaders in that program
>> + * that have a static use gl_FragCoord. All redeclarations of
>> + * gl_FragCoord in all fragment shaders in a single program must have
>> + * the same set of qualifiers."
>> + */
>> +
>> + for (unsigned i = 0; i < num_shaders; i++) {
>> + struct gl_shader *shader = shader_list[i];
>> +
>> + if (fs_uses_conflicting_layout_qualifiers(shader, linked_shader)) {
>> + linker_error(prog, "fragment shader defined with conflicting "
>> + "layout qualifiers for gl_FragCoord\n");
>> + return;
>> + } else if (shader->redeclares_gl_fragcoord || shader->uses_gl_fragcoord) {
>> + linked_shader->uses_gl_fragcoord = shader->uses_gl_fragcoord;
>> + linked_shader->origin_upper_left = shader->origin_upper_left;
>> + linked_shader->pixel_center_integer = shader->pixel_center_integer;
>> + }
>> + }
>> +}
>> +
>> /**
>> * Performs the cross-validation of geometry shader max_vertices and
>> * primitive type layout qualifiers for the attached geometry shaders,
>> @@ -1471,6 +1530,7 @@ link_intrastage_shaders(void *mem_ctx,
>> linked->NumUniformBlocks = num_uniform_blocks;
>> ralloc_steal(linked, linked->UniformBlocks);
>>
>> + link_fs_input_layout_qualifiers(prog, linked, shader_list, num_shaders);
>> link_gs_inout_layout_qualifiers(prog, linked, shader_list, num_shaders);
>> link_cs_input_layout_qualifiers(prog, linked, shader_list, num_shaders);
>>
>> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
>> index fbbca55..dd2ee82 100644
>> --- a/src/mesa/main/mtypes.h
>> +++ b/src/mesa/main/mtypes.h
>> @@ -2414,6 +2414,14 @@ struct gl_shader
>> struct glsl_symbol_table *symbols;
>>
>> bool uses_builtin_functions;
>> + bool uses_gl_fragcoord;
>> + bool redeclares_gl_fragcoord;
>> +
>> + /**
>> + * Fragment shader state from GLSL 1.50 layout qualifiers.
>> + */
>> + bool origin_upper_left;
>> + bool pixel_center_integer;
>>
>> /**
>> * Geometry shader state from GLSL 1.50 layout qualifiers.
>>
>
More information about the mesa-dev
mailing list