[Mesa-dev] [PATCH 07/13] linker: Use gl_shader_program::AttributeBindings for attrib locations
Ian Romanick
idr at freedesktop.org
Fri Oct 7 11:09:25 PDT 2011
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.
Do you have any other insights?
> 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