[Mesa-dev] [PATCH 07/13] linker: Use gl_shader_program::AttributeBindings for attrib locations
Stéphane Marchesin
stephane.marchesin at gmail.com
Fri Oct 7 11:20:34 PDT 2011
2011/10/7 Ian Romanick <idr at freedesktop.org>:
> On 10/06/2011 06:54 PM, Stéphane Marchesin wrote:
>>
>> Hi Ian,
>>
>> This regresses Chrome GPU acceleration for all GPUs (I tested i915g,
>> llvmpipe, i965).
>
> See also bugzilla #41499 and #41508. I tried to debug this a little
> yesterday, but I couldn't see what was going wrong. None of our piglit or
> GLES2 tests regress, so that's annoying.
>
> Looking at the misrendered images, it looks like the attributes are just
> getting sent to the wrong slots. All that I can figure is that the failing
> programs link the program, set the attribute bindings, and then re-link the
> program. Somehow in that process the bindings set by the linker in the
> first link are used instead of the ones set by the application (and the
> second link). As far as I can tell, that should work, but I haven't had a
> chance to put together a test case.
>
Isn't it enough to run Chrome? It reproduces 100%.
> Do you have any other insights?
I didn't really look into it, I got the same graphical result as
41508, and bisected to that commit.
Stéphane
>
>> Stéphane
>>
>>
>> On Fri, Sep 30, 2011 at 16:44, Ian Romanick<idr at freedesktop.org> wrote:
>>>
>>> From: Ian Romanick<ian.d.romanick at intel.com>
>>>
>>> Signed-off-by: Ian Romanick<ian.d.romanick at intel.com>
>>> ---
>>> src/glsl/linker.cpp | 138
>>> +++++++++++++++++++++++---------------------------
>>> 1 files changed, 64 insertions(+), 74 deletions(-)
>>>
>>> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
>>> index 6d40dd2..9463f53 100644
>>> --- a/src/glsl/linker.cpp
>>> +++ b/src/glsl/linker.cpp
>>> @@ -1298,71 +1298,6 @@
>>> assign_attribute_or_color_locations(gl_shader_program *prog,
>>>
>>> invalidate_variable_locations(sh, direction, generic_base);
>>>
>>> - if ((target_index == MESA_SHADER_VERTEX)&& (prog->Attributes !=
>>> NULL)) {
>>> - for (unsigned i = 0; i< prog->Attributes->NumParameters; i++) {
>>> - ir_variable *const var =
>>> -
>>> sh->symbols->get_variable(prog->Attributes->Parameters[i].Name);
>>> -
>>> - /* Note: attributes that occupy multiple slots, such as arrays
>>> or
>>> - * matrices, may appear in the attrib array multiple times.
>>> - */
>>> - if ((var == NULL) || (var->location != -1))
>>> - continue;
>>> -
>>> - /* From page 61 of the OpenGL 4.0 spec:
>>> - *
>>> - * "LinkProgram will fail if the attribute bindings assigned
>>> by
>>> - * BindAttribLocation 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."
>>> - *
>>> - * Previous versions of the spec contain similar language but
>>> omit the
>>> - * bit about attribute arrays.
>>> - *
>>> - * Page 61 of the OpenGL 4.0 spec also says:
>>> - *
>>> - * "It is possible for an application to bind more than one
>>> - * attribute name to the same location. This is referred to
>>> as
>>> - * aliasing. 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
>>> - * 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
>>> attribute
>>> - * FINISHME: location aliasing (see comment above).
>>> - */
>>> - const int attr =
>>> prog->Attributes->Parameters[i].StateIndexes[0];
>>> - const unsigned slots = count_attribute_slots(var->type);
>>> -
>>> - /* Mask representing the contiguous slots that will be used by
>>> this
>>> - * attribute.
>>> - */
>>> - const unsigned use_mask = (1<< slots) - 1;
>>> -
>>> - /* 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) {
>>> - linker_error(prog,
>>> - "insufficient contiguous attribute locations "
>>> - "available for vertex shader input `%s'",
>>> - var->name);
>>> - return false;
>>> - }
>>> -
>>> - var->location = VERT_ATTRIB_GENERIC0 + attr;
>>> - used_locations |= (use_mask<< attr);
>>> - }
>>> - }
>>> -
>>> /* Temporary storage for the set of attributes that need locations
>>> assigned.
>>> */
>>> struct temp_attr {
>>> @@ -1389,28 +1324,83 @@
>>> assign_attribute_or_color_locations(gl_shader_program *prog,
>>> continue;
>>>
>>> if (var->explicit_location) {
>>> - const unsigned slots = count_attribute_slots(var->type);
>>> - const unsigned use_mask = (1<< slots) - 1;
>>> - const int attr = var->location - generic_base;
>>> -
>>> if ((var->location>= (int)(max_index + generic_base))
>>> || (var->location< 0)) {
>>> linker_error(prog,
>>> "invalid explicit location %d specified for
>>> `%s'\n",
>>> - (var->location< 0) ? var->location : attr,
>>> + (var->location< 0)
>>> + ? var->location : var->location - generic_base,
>>> var->name);
>>> return false;
>>> - } else if (var->location>= generic_base) {
>>> - used_locations |= (use_mask<< attr);
>>> + }
>>> + } else if (target_index == MESA_SHADER_VERTEX) {
>>> + unsigned binding;
>>> +
>>> + if (prog->AttributeBindings->get(binding, var->name)) {
>>> + assert(binding>= VERT_ATTRIB_GENERIC0);
>>> + var->location = binding;
>>> }
>>> }
>>>
>>> /* The location was explicitly assigned, nothing to do here.
>>> */
>>> - if (var->location != -1)
>>> + const unsigned slots = count_attribute_slots(var->type);
>>> + if (var->location != -1) {
>>> + if (var->location>= generic_base) {
>>> + /* From page 61 of the OpenGL 4.0 spec:
>>> + *
>>> + * "LinkProgram will fail if the attribute bindings
>>> assigned
>>> + * by BindAttribLocation 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."
>>> + *
>>> + * Previous versions of the spec contain similar language but
>>> omit
>>> + * the bit about attribute arrays.
>>> + *
>>> + * Page 61 of the OpenGL 4.0 spec also says:
>>> + *
>>> + * "It is possible for an application to bind more than
>>> one
>>> + * attribute name to the same location. This is referred
>>> to as
>>> + * aliasing. 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 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).
>>> + */
>>> + /* Mask representing the contiguous slots that will be used
>>> by
>>> + * this attribute.
>>> + */
>>> + const unsigned attr = var->location - generic_base;
>>> + const unsigned use_mask = (1<< slots) - 1;
>>> +
>>> + /* 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)
>>> {
>>> + linker_error(prog,
>>> + "insufficient contiguous attribute locations
>>> "
>>> + "available for vertex shader input `%s'",
>>> + var->name);
>>> + return false;
>>> + }
>>> +
>>> + used_locations |= (use_mask<< attr);
>>> + }
>>> +
>>> continue;
>>> + }
>>>
>>> - to_assign[num_attr].slots = count_attribute_slots(var->type);
>>> + to_assign[num_attr].slots = slots;
>>> to_assign[num_attr].var = var;
>>> num_attr++;
>>> }
>>> --
>>> 1.7.6
>>>
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>>
>>
>
>
More information about the mesa-dev
mailing list