On Fri, Apr 18, 2014 at 4:11 PM, Ian Romanick <i...@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.pho...@gmail.com>
>> Cc: "9.2 10.0 10.1" <mesa-sta...@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);
>>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
