[Mesa-stable] [Mesa-dev] [PATCH] glsl: set the binding value regardless explicit_binding

[Alejandro Piñeiro]

On 03/06/15 13:17, Emil Velikov wrote:
> Hello gents,
>
> On 21 May 2015 at 14:03, Timothy Arceri <t_arceri at yahoo.com.au> wrote:
>> On Thu, 2015-05-21 at 12:22 +0200, Alejandro Piñeiro wrote:
>>> On 20/05/15 23:39, Timothy Arceri wrote:
>>>> On Thu, 2015-05-14 at 22:49 +0200, Alejandro Piñeiro wrote:
>>>>> On 14/05/15 20:38, Ian Romanick wrote:
>>>>>> I think I see what's happening.  I don't think I understood
>>>>>> atomic.buffer_index well enough when I removed it. :(  In binding=3
>>>>>> case, what value does
>>>>>>
>>>>>>     glGetActiveAtomicCounterBufferiv(prog,
>>>>>>                                      0,
>>>>>>                                      GL_ATOMIC_COUNTER_BUFFER_BINDING,
>>>>>>                                      param);
>>>>>>
>>>>>> give?  My guess is that it gives 0 instead of the binding specified in
>>>>>> the shader.
>>>>> With the test I uploaded today, so with binding points 3 and 6, and
>>>>> using index 0 and 1, calling glGetActiveAtomicCounterBufferiv properly
>>>>> returns 3 and 6. Both with and without the patch of this thread.
>>>> The correct binding is stored in the gl_active_atomic_buffer struct
>>>> which queried by glGetActiveAtomicCounterBufferiv()
>>> Ok.
>>>
>>>
>>>>>>   This would be a good addition to the test.
>>>>> Ok.
>>>>>
>>>>>> The index and the binding are different.  The index is "which atomic is
>>>>>> this in the shader," and the binding is "which binding point is used to
>>>>>> attach a buffer to this atomic."  Thanks to c0cd5b, I think somewhere
>>>>>> we're using one value when we actually want the other... probably the
>>>>>> last hunk:
>>>>>>
>>>>>> --- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
>>>>>> +++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
>>>>>> @@ -2294,7 +2294,7 @@
>>>>>> vec4_visitor::visit_atomic_counter_intrinsic(ir_call *ir)
>>>>>>        ir->actual_parameters.get_head());
>>>>>>     ir_variable *location = deref->variable_referenced();
>>>>>>     unsigned surf_index = (prog_data->base.binding_table.abo_start +
>>>>>> -                          location->data.atomic.buffer_index);
>>>>>> +                          location->data.binding);
>>>>>>
>>>>>>     /* Calculate the surface offset */
>>>>>>     src_reg offset(this, glsl_type::uint_type);
>>>>>>
>>>>>> Maybe try adding a utility function that will search
>>>>>> gl_shader_program::UniformStorage for location->name and use the
>>>>>> atomic_buffer_index stored there for use in the i965 driver?
>>>>> Taking into account that glGetActiveAtomicCounterBufferiv is already
>>>>> working, do you think that this is still needed?
>>>> For clarity its probably a good idea to implement Ian's suggestion.
>>>> Changing the binding value to the atomic buffer index (even though it
>>>> seems to be unused elsewhere after this point) is a bit confusing. This
>>>> email conversation is a good enough example of the confusion doing this
>>>> can cause.
>>>>
>>>> If you implement Ian's suggestion you can probably remove the if
>>>> statement too as the binding value is not used after this point.
>>> Probably I'm missing a global view of the situation, but taking into
>>> account that one of the conclusions of this email conversation is that
>>> the index and the binding are different, and removing one of the
>>> variables from ir_variable::data.atomic is causing some confusion (and
>>> the regression), shouldn't it easier to just revert the change? Commit
>>> c0cd5b explains that the removal helped to reduce memory consumption,
>>> but as far as I understand, it was done because it was concluded that
>>> was not needed.
>>
>> It was part of a memory reduction series, so I'm not sure it should be
>> reverted. I'm sure Ian would have suggested doing so if he thought that
>> was the better option:
>>
>> http://lists.freedesktop.org/archives/mesa-dev/2014-July/063303.html
>>
>>
>>>> Although I did notice this in the glsl to nir nir code:
>>>>
>>>>    /* XXX Get rid of buffer_index */
>>>>    var->data.atomic.buffer_index = ir->data.binding;
>>>>
>>>> So this may be a problem too..
>>> Well, if the change is reverted, it is just about removing that comment
>>> (assuming that the comment was added for the same reason commit c0cd5b
>>> was done).
>> If the change was to be reverted this would probably need to be changed
>> to
>>
>> var->data.atomic.buffer_index = ir->data.atomic.buffer_index;
>>
>> as nir didn't exist when the change was made.
>>
> In light of the above discussion should we consider this patch as
> obsolete (more of incomplete iiuc) or is it still applicable ? 

Yes, as far as I understood Ian and Timothy comments, they prefer to
create a new patch, in order to solve the regression, but keeping the
memory footprint gain. So yes, my patch(es) can be considered obsolete.
Right now I don't have time to resume the work on this issue, but I
would do that as soon as I have some time for that.

Best regards

-- 
Alejandro Piñeiro (apinheiro at igalia.com)