[Mesa-stable] [PATCH] glsl: Allow overlapping locations for vertex input attributes
Anuj Phogat
anuj.phogat at gmail.com
Mon Mar 24 16:42:42 PDT 2014
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.phogat at gmail.com> wrote:
>
>
>
> On Tue, Mar 11, 2014 at 10:37 AM, Ian Romanick <idr at freedesktop.org> wrote:
>>
>> On 03/10/2014 04:15 PM, Anuj Phogat wrote:
>> >
>> >
>> >
>> > On Mon, Mar 10, 2014 at 3:27 PM, Ian Romanick <idr at freedesktop.org
>> > <mailto:idr at 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.phogat at gmail.com
>> > <mailto:anuj.phogat at gmail.com>>
>> > > Cc: "9.2 10.0 10.1" <mesa-stable at lists.freedesktop.org
>> > <mailto:mesa-stable at 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);
>> >
>> >
>>
>
More information about the mesa-stable
mailing list