[Mesa-dev] [Mesa-stable] [PATCH V2] glsl: Allow overlapping locations for vertex input attributes
Ian Romanick
idr at freedesktop.org
Fri Apr 18 16:11:07 PDT 2014
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 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?
> */
> +
> /* 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?
> + 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-dev
mailing list