[Mesa-dev] [PATCH 07/13] linker: Use gl_shader_program::AttributeBindings for attrib locations
Ian Romanick
idr at freedesktop.org
Fri Oct 7 12:21:32 PDT 2011
On 10/07/2011 11:20 AM, Stéphane Marchesin wrote:
> 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%.
I guess I should have said "minimal test case." :) I can't really put
Chrome in piglit. However, it looks like the existing
glsl-bindattriblocation tests should hit the case I was worrying about.
>> 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
More information about the mesa-dev
mailing list