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.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? > */ > + > /* 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); > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev