On Tue, Feb 25, 2014 at 5:02 PM, Anuj Phogat <anuj.pho...@gmail.com> wrote:
> On Tue, Feb 25, 2014 at 9:14 AM, Ian Romanick <i...@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.pho...@gmail.com>
>>> Cc: <mesa-sta...@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) {
Found another failing case. Making corrections to my previous comment.
This exception should go along with other if conditions above:
if ((linked_shader->redeclares_gl_fragcoord
&& !shader->redeclares_gl_fragcoord
&& shader->uses_gl_fragcoord
&& (linked_shader->origin_upper_left
|| linked_shader->pixel_center_integer))
|| (shader->redeclares_gl_fragcoord
&& !linked_shader->redeclares_gl_fragcoord
&& linked_shader->uses_gl_fragcoord)) {
//error
}
> // 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;
correction:
linked_shader->redeclares_gl_fragcoord = shader->redeclares_gl_fragcoord;
>
> 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.
>>>
>>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
