Sent out a V2 of this patch which fixes a bug I noticed recently.
On Thu, Mar 13, 2014 at 3:39 PM, Anuj Phogat <anuj.pho...@gmail.com> wrote: > > > > On Tue, Mar 11, 2014 at 10:37 AM, Ian Romanick <i...@freedesktop.org> wrote: >> >> On 03/10/2014 04:15 PM, Anuj Phogat wrote: >> > >> > >> > >> > On Mon, Mar 10, 2014 at 3:27 PM, Ian Romanick <i...@freedesktop.org >> > <mailto:i...@freedesktop.org>> wrote: >> > >> > On 03/10/2014 11:19 AM, 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. >> > > >> > > Signed-off-by: Anuj Phogat <anuj.pho...@gmail.com >> > <mailto:anuj.pho...@gmail.com>> >> > > Cc: "9.2 10.0 10.1" <mesa-sta...@lists.freedesktop.org >> > <mailto:mesa-sta...@lists.freedesktop.org>> >> > > Bugzilla: Khronos #9609 >> > > --- >> > > src/glsl/linker.cpp | 74 >> > +++++++++++++++++++++++++++++++++++++++++++---------- >> > > 1 file changed, 61 insertions(+), 13 deletions(-) >> > > >> > > diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp >> > > index a619bc8..9bd698b 100644 >> > > --- a/src/glsl/linker.cpp >> > > +++ b/src/glsl/linker.cpp >> > > @@ -1798,10 +1798,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. >> > >> > The text above (outside the diff) forbids things like: >> > >> > layout(location=0) in mat4 m; >> > layout(location=1) in vec4 v; >> > >> > where v and m now partially overlap because m consumes 4 slots (0 >> > through 3, inclusive). >> > >> > >> > This should not be a problem as along as there's only one active >> > attribute >> > in vertex shader. If a developer uses such locations, its his >> > responsibility >> > to use just one of them in any path in shader program. Khronos CTS test >> > also expects linking to happen in above case. >> > >> > OpenGL 4.0 spec makes similar looking statements at two different >> > places. >> > First for vertex shader input locations at page 60: >> > >> > "LinkProgram will fail if the attribute bindings assigned by BindAttri- >> > bLocation do not leave not enough space to assign a location for an >> > active matrix >> > attribute or an active attribute array, both of which require multiple >> > contiguous >> > generic attributes." >> >> Somewhere in the intervening specs, the language was changed. In 4.4 it >> says: >> >> "LinkProgram will fail if the attribute bindings specified either >> by BindAttribLocation or explicitly set within the shader text do >> not leave not enough space to assign a location for an active >> matrix attribute or an active attribute array, both of which >> require multiple contiguous generic attributes." >> >> > This statement sounds little unclear to me. It doesn't say what happens >> > if >> > locations set in shader text don't leave enough space. It also doesn't >> > make >> > clear who assigns the location in "to assign a location for an active >> > matrix". >> > explicit or automatic assignment? >> >> You may be right. We'll have to prod other implementations. I'll also >> do some spec archaeology. > > Piglit tests I posted on mailing list passes on NVIDIA's OpenGL 4.3 linux > driver. NVIDIA allows the kind of overlapping you described above. >> >> >> > Second for fragment output locations at page 233: >> > "if the explicit binding assigments do not leave enough space for the >> > linker >> > to automatically assign a location for a varying out array, which >> > requires >> > multiple contiguous locations." >> > >> > This statement makes the condition more clear for linking error. Notice >> > the words >> > 'explicit' and 'automatically' in text from page 233. >> > >> > So I interpreted the two statements as: >> > Aliasing of explicit and automatic assignments is prohibited. >> > But, aliasing is allowed in explicit assignments of attribute locations. >> > >> > Do you interpret them differently ? >> > >> > >> > Does this patch affect this? >> > >> > Yes. It allows linking in such cases. >> > >> > >> > > * >> > > - * 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,12 +1816,48 @@ >> > 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. >> > >> > These last two paragraphis should be un-indented. >> > >> > > */ >> > > + >> > > /* Mask representing the contiguous slots that will be >> > used by >> > > * this attribute. >> > > */ >> > > @@ -1832,11 +1870,21 @@ >> > assign_attribute_or_color_locations(gl_shader_program *prog, >> > > 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
