[Mesa-stable] [PATCH V2] glsl: Allow overlapping locations for vertex input attributes

Anuj Phogat anuj.phogat at gmail.com
Mon Apr 21 14:24:41 PDT 2014


On Fri, Apr 18, 2014 at 4:11 PM, Ian Romanick <idr at freedesktop.org> wrote:
> On 03/24/2014 04:33 PM, Anuj Phogat wrote:
>> Currently overlapping locations of input variables are not allowed for all
>> the shader types in OpenGL and OpenGL ES.
>>
>>>From OpenGL ES 3.0 spec, page 56:
>>    "Binding more than one attribute name to the same location is referred
>>     to as aliasing, and is not permitted in OpenGL ES Shading Language
>>     3.00 vertex shaders. LinkProgram will fail when this condition exists.
>>     However, aliasing is possible in OpenGL ES Shading Language 1.00 vertex
>>     shaders."
>>
>> Taking in to account what different versions of OpenGL and OpenGL ES specs
>> say about aliasing:
>>    - It is allowed only on vertex shader input attributes in OpenGL (2.0 and
>>      above) and OpenGL ES 2.0.
>>    - It is explictly disallowed in OpenGL ES 3.0.
>>
>> Fixes Khronos CTS failing test:
>> explicit_attrib_location_vertex_input_aliased.test
>> See more details about this at below mentioned khronos bug.
>>
>> V2: Fix the case where location exceeds the maximum allowed attribute
>> location.
>
> I made a couple comments below.
>
> I also have a bigger worry about this.  What does the i965 backend (or
> other backends) do with a shader like:
>
>     layout(location=0) vec4 a;
>     layout(location=0) ivec2 b;
>     out vec4 p;
>     out ivec2 q;
>
>     void main()
>     {
>         p = a;
>         q = b;
>         gl_Position = vec4(0);
>     }
>
> Or, a shader that's hypothetically valid:
>
>     layout(location=0) vec4 a;
>     layout(location=0) ivec2 b;
>     uniform bool b;
>     out vec4 p;
>
>     void main()
>     {
>         p = b ? a : vec4(b, 0, 0);
>         gl_Position = vec4(0);
>     }
>
I have added a piglit test using similar vertex shader. Test passes
with this patch.
Piglit test: http://patchwork.freedesktop.org/patch/21859/

I can add few more tests if that's not sufficient.
> I have this sinking feeling that the backend will explode in some way.
>
>> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
>> Cc: "9.2 10.0 10.1" <mesa-stable at lists.freedesktop.org>
>> Bugzilla: Khronos #9609
>> ---
>>  src/glsl/linker.cpp | 91 ++++++++++++++++++++++++++++++++++++++++++++---------
>>  1 file changed, 76 insertions(+), 15 deletions(-)
>>
>> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
>> index 8019444..13947ce 100644
>> --- a/src/glsl/linker.cpp
>> +++ b/src/glsl/linker.cpp
>> @@ -1692,6 +1692,9 @@ assign_attribute_or_color_locations(gl_shader_program *prog,
>>     unsigned used_locations = (max_index >= 32)
>>        ? ~0 : ~((1 << max_index) - 1);
>>
>> +   /* Initially both numbers are equal. */
>> +   unsigned available_locations = used_locations;
>> +
>>     assert((target_index == MESA_SHADER_VERTEX)
>>         || (target_index == MESA_SHADER_FRAGMENT));
>>
>> @@ -1798,10 +1801,12 @@ assign_attribute_or_color_locations(gl_shader_program *prog,
>>            *     active attribute array, both of which require multiple
>>            *     contiguous generic attributes."
>>            *
>> -          * Previous versions of the spec contain similar language but omit
>> -          * the bit about attribute arrays.
>> +          * I think above text prohibits the aliasing of explicit and
>> +          * automatic assignments. But, aliasing is allowed in manual
>> +          * assignments of attribute locations. See below comments for
>> +          * the details.
>>            *
>> -          * Page 61 of the OpenGL 4.0 spec also says:
>> +          * From OpenGL 4.0 spec, page 61:
>>            *
>>            *     "It is possible for an application to bind more than one
>>            *     attribute name to the same location. This is referred to as
>> @@ -1814,29 +1819,85 @@ assign_attribute_or_color_locations(gl_shader_program *prog,
>>            *     but implementations are not required to generate an error
>>            *     in this case."
>>            *
>> -          * These two paragraphs are either somewhat contradictory, or I
>> -          * don't fully understand one or both of them.
>> -          */
>> -         /* FINISHME: The code as currently written does not support
>> -          * FINISHME: attribute location aliasing (see comment above).
>> +          * From GLSL 4.30 spec, page 54:
>> +          *
>> +          *    "A program will fail to link if any two non-vertex shader
>> +          *     input variables are assigned to the same location. For
>> +          *     vertex shaders, multiple input variables may be assigned
>> +          *     to the same location using either layout qualifiers or via
>> +          *     the OpenGL API. However, such aliasing is intended only to
>> +          *     support vertex shaders where each execution path accesses
>> +          *     at most one input per each location. Implementations are
>> +          *     permitted, but not required, to generate link-time errors
>> +          *     if they detect that every path through the vertex shader
>> +          *     executable accesses multiple inputs assigned to any single
>> +          *     location. For all shader types, a program will fail to link
>> +          *     if explicit location assignments leave the linker unable
>> +          *     to find space for other variables without explicit
>> +          *     assignments."
>> +          *
>> +          * From OpenGL ES 3.0 spec, page 56:
>> +          *
>> +          *    "Binding more than one attribute name to the same location
>> +          *     is referred to as aliasing, and is not permitted in OpenGL
>> +          *     ES Shading Language 3.00 vertex shaders. LinkProgram will
>> +          *     fail when this condition exists. However, aliasing is
>> +          *     possible in OpenGL ES Shading Language 1.00 vertex shaders.
>> +          *     This will only work if only one of the aliased attributes
>> +          *     is active in the executable program, or if no path through
>> +          *     the shader consumes more than one attribute of a set of
>> +          *     attributes aliased to the same location. A link error can
>> +          *     occur if the linker determines that every path through the
>> +          *     shader consumes multiple aliased attributes, but implemen-
>> +          *     tations are not required to generate an error in this case."
>> +          *
>> +          *     After looking at above references from OpenGL, OpenGL ES
>> +          *     and GLSL specifications, we allow aliasing of vertex input
>> +          *     variables in: OpenGL 2.0 (and above) and OpenGL ES 2.0.
>> +          *
>> +          *     NOTE: This is not required by the spec but its worth
>> +          *     mentioning here that we're not doing anything to make sure
>> +          *     that no path through the vertex shader executable accesses
>> +          *     multiple inputs assigned to any single location.
>
> The last two paragraphs shouldn't be indented... because they're not
> part of the spec quote, right?
right.
>
>>            */
>> +
>>           /* Mask representing the contiguous slots that will be used by
>>            * this attribute.
>>            */
>>           const unsigned attr = var->data.location - generic_base;
>>           const unsigned use_mask = (1 << slots) - 1;
>> +            const char *const string = (target_index == MESA_SHADER_VERTEX)
>> +               ? "vertex shader input" : "fragment shader output";
>> +
>> +            /* Generate a link error if the requested locations for this
>> +             * attribute exceed the maximum allowed attribute location.
>> +             */
>> +            if ((~(use_mask << attr) & available_locations)
>> +                != available_locations) {
>
> I think this could just be
>
>       if (attr + slots > max_index)
>
> Because available_locations starts as all of the possible locations, and
> it is never updated.  Right?
Yes. This is much simpler. I'll make the suggested change.

>
>> +               linker_error(prog,
>> +                           "insufficient contiguous locations "
>> +                           "available for %s `%s' %d %d %d", string,
>> +                           var->name, used_locations, use_mask, attr);
>> +               return false;
>> +            }
>>
>>           /* Generate a link error if the set of bits requested for this
>>            * attribute overlaps any previously allocated bits.
>>            */
>>           if ((~(use_mask << attr) & used_locations) != used_locations) {
>> -            const char *const string = (target_index == MESA_SHADER_VERTEX)
>> -               ? "vertex shader input" : "fragment shader output";
>> -            linker_error(prog,
>> -                         "insufficient contiguous locations "
>> -                         "available for %s `%s' %d %d %d", string,
>> -                         var->name, used_locations, use_mask, attr);
>> -            return false;
>> +               if (target_index == MESA_SHADER_FRAGMENT ||
>> +                   (prog->IsES && prog->Version >= 300)) {
>> +                  linker_error(prog,
>> +                               "overlapping location is assigned "
>> +                               "to %s `%s' %d %d %d\n", string,
>> +                               var->name, used_locations, use_mask, attr);
>> +                  return false;
>> +               } else {
>> +                  linker_warning(prog,
>> +                                 "overlapping location is assigned "
>> +                                 "to %s `%s' %d %d %d\n", string,
>> +                                 var->name, used_locations, use_mask, attr);
>> +               }
>>           }
>>
>>           used_locations |= (use_mask << attr);
>>
>


More information about the mesa-stable mailing list