Re: [Mesa-dev] [PATCH V3] glsl: fix atomic buffer index for bindings other than 0

[Timothy Arceri]

On Tue, 2015-09-01 at 14:46 +0100, Emil Velikov wrote:
> On 29 July 2015 at 13:46, Timothy Arceri  wrote:
> > On Wed, 2015-07-29 at 09:57 +0200, Iago Toral wrote:
> > > On Sun, 2015-07-26 at 18:35 +1000, Timothy Arceri wrote:
> > > > Since commit c0cd5b var->data.binding was being used as a replacement
> > > > for atomic buffer index, but they don't have to be the same value they
> > > > just happen to end up the same when binding is 0.
> > > > 
> > > > Now we store atomic buffer index in the unused var->data.index
> > > > to avoid the extra memory of putting back the atmoic buffer index 
> > > > field.
> > > 
> > > Could this be a bit too restrictive? var->data.index has only a single
> > > bit of storage, so this would limit the number of buffers we can index
> > > to 2.
> > 
> > Your right I wasn't paying enough attention, the nir struct doesn't place 
> > the
> > same limits on index (although maybe it should) and I didn't notice it in 
> > the
> > glsl ir.
> > 
> > I have a new solution on the way as part on V3 of my AoA work, however its 
> > not
> > really suitable for stable.
> > 
> > If we want this fix in stable maybe putting back the atomic_index struct
> > member is the best solution after all.
> > 
> I guess we can/should drop this from the queue, or is it something
> still worthy for stable ?
> If so, can anyone let me know of the requirements (commit name and/or
> sha should be great).
> 

Yeah drop this. Soory for the noise, I've got a better fix as part of the AoA
work but its maybe too invasive for stable.

> Thanks,
> Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH V3] glsl: fix atomic buffer index for bindings other than 0

[Emil Velikov]

On 29 July 2015 at 13:46, Timothy Arceri  wrote:
> On Wed, 2015-07-29 at 09:57 +0200, Iago Toral wrote:
>> On Sun, 2015-07-26 at 18:35 +1000, Timothy Arceri wrote:
>> > Since commit c0cd5b var->data.binding was being used as a replacement
>> > for atomic buffer index, but they don't have to be the same value they
>> > just happen to end up the same when binding is 0.
>> >
>> > Now we store atomic buffer index in the unused var->data.index
>> > to avoid the extra memory of putting back the atmoic buffer index field.
>>
>> Could this be a bit too restrictive? var->data.index has only a single
>> bit of storage, so this would limit the number of buffers we can index
>> to 2.
>
> Your right I wasn't paying enough attention, the nir struct doesn't place the
> same limits on index (although maybe it should) and I didn't notice it in the
> glsl ir.
>
> I have a new solution on the way as part on V3 of my AoA work, however its not
> really suitable for stable.
>
> If we want this fix in stable maybe putting back the atomic_index struct
> member is the best solution after all.
>
I guess we can/should drop this from the queue, or is it something
still worthy for stable ?
If so, can anyone let me know of the requirements (commit name and/or
sha should be great).

Thanks,
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH V3] glsl: fix atomic buffer index for bindings other than 0

[Iago Toral]

On Sun, 2015-07-26 at 18:35 +1000, Timothy Arceri wrote:
 Since commit c0cd5b var-data.binding was being used as a replacement
 for atomic buffer index, but they don't have to be the same value they
 just happen to end up the same when binding is 0.
 
 Now we store atomic buffer index in the unused var-data.index
 to avoid the extra memory of putting back the atmoic buffer index field.

Could this be a bit too restrictive? var-data.index has only a single
bit of storage, so this would limit the number of buffers we can index
to 2. The spec says that the minimum an implementation should support is
1, so it accomplishes that, but still... a quick look through the i965
code reveals that it is prepared to support up to 16 for example (see
BRW_MAX_ABO in brw_context.h).

Iago

 V3: Dont make unrelated changes to the uniform storage handling.
 Cc the correct stable branch.
 
 V2: store buffer index in var-data.index and uniform slot in
 var-data.location to avoid issues when linking more than 2 shaders.
 Also some small tidy ups.
 
 Cc: Alejandro Piñeiro apinhe...@igalia.com
 Cc: Ian Romanick i...@freedesktop.org
 Cc: 10.6 mesa-sta...@lists.freedesktop.org
 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90175
 ---
  src/glsl/ir.h  | 2 ++
  src/glsl/link_atomics.cpp  | 2 ++
  src/glsl/nir/glsl_to_nir.cpp   | 2 --
  src/glsl/nir/nir.h | 5 +++--
  src/glsl/nir/nir_lower_atomics.c   | 2 +-
  src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 2 +-
  6 files changed, 9 insertions(+), 6 deletions(-)
 
 diff --git a/src/glsl/ir.h b/src/glsl/ir.h
 index ede8caa..f77534c 100644
 --- a/src/glsl/ir.h
 +++ b/src/glsl/ir.h
 @@ -757,6 +757,8 @@ public:
 * \note
 * The GLSL spec only allows the values 0 or 1 for the index in \b dual
 * source blending.
 +   *
 +   * For atomic counters this stores the atomic buffer index.
 */
unsigned index:1;
  
 diff --git a/src/glsl/link_atomics.cpp b/src/glsl/link_atomics.cpp
 index 100d03c..5120564 100644
 --- a/src/glsl/link_atomics.cpp
 +++ b/src/glsl/link_atomics.cpp
 @@ -204,6 +204,8 @@ link_assign_atomic_counter_resources(struct gl_context 
 *ctx,
   if (!var-data.explicit_binding)
  var-data.binding = i;
  
 + var-data.index = i;
 +
   storage-atomic_buffer_index = i;
   storage-offset = var-data.atomic.offset;
   storage-array_stride = (var-type-is_array() ?
 diff --git a/src/glsl/nir/glsl_to_nir.cpp b/src/glsl/nir/glsl_to_nir.cpp
 index 77327b6..6cb23c0 100644
 --- a/src/glsl/nir/glsl_to_nir.cpp
 +++ b/src/glsl/nir/glsl_to_nir.cpp
 @@ -326,8 +326,6 @@ nir_visitor::visit(ir_variable *ir)
  
 var-data.index = ir-data.index;
 var-data.binding = ir-data.binding;
 -   /* XXX Get rid of buffer_index */
 -   var-data.atomic.buffer_index = ir-data.binding;
 var-data.atomic.offset = ir-data.atomic.offset;
 var-data.image.read_only = ir-data.image_read_only;
 var-data.image.write_only = ir-data.image_write_only;
 diff --git a/src/glsl/nir/nir.h b/src/glsl/nir/nir.h
 index 62cdbd4..837d197 100644
 --- a/src/glsl/nir/nir.h
 +++ b/src/glsl/nir/nir.h
 @@ -292,7 +292,9 @@ typedef struct {
unsigned int driver_location;
  
/**
 -   * output index for dual source blending.
 +   * Output index for dual source blending.
 +   *
 +   * For atomic counters this stores the atomic buffer index.
 */
int index;
  
 @@ -307,7 +309,6 @@ typedef struct {
 * Location an atomic counter is stored at.
 */
struct {
 - unsigned buffer_index;
   unsigned offset;
} atomic;
  
 diff --git a/src/glsl/nir/nir_lower_atomics.c 
 b/src/glsl/nir/nir_lower_atomics.c
 index ce3615a..6119f62 100644
 --- a/src/glsl/nir/nir_lower_atomics.c
 +++ b/src/glsl/nir/nir_lower_atomics.c
 @@ -63,7 +63,7 @@ lower_instr(nir_intrinsic_instr *instr, nir_function_impl 
 *impl)
  
 nir_intrinsic_instr *new_instr = nir_intrinsic_instr_create(mem_ctx, op);
 new_instr-const_index[0] =
 -  (int) instr-variables[0]-var-data.atomic.buffer_index;
 +  (int) instr-variables[0]-var-data.index;
  
 nir_load_const_instr *offset_const = nir_load_const_instr_create(mem_ctx, 
 1);
 offset_const-value.u[0] = instr-variables[0]-var-data.atomic.offset;
 diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp 
 b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
 index 6fee798..5e0bf1b 100644
 --- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
 +++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
 @@ -2407,7 +2407,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.binding);
 + 

Re: [Mesa-dev] [PATCH V3] glsl: fix atomic buffer index for bindings other than 0

[Timothy Arceri]

On Wed, 2015-07-29 at 09:57 +0200, Iago Toral wrote:
 On Sun, 2015-07-26 at 18:35 +1000, Timothy Arceri wrote:
  Since commit c0cd5b var-data.binding was being used as a replacement
  for atomic buffer index, but they don't have to be the same value they
  just happen to end up the same when binding is 0.
  
  Now we store atomic buffer index in the unused var-data.index
  to avoid the extra memory of putting back the atmoic buffer index field.
 
 Could this be a bit too restrictive? var-data.index has only a single
 bit of storage, so this would limit the number of buffers we can index
 to 2.

Your right I wasn't paying enough attention, the nir struct doesn't place the
same limits on index (although maybe it should) and I didn't notice it in the
glsl ir.

I have a new solution on the way as part on V3 of my AoA work, however its not
really suitable for stable.

If we want this fix in stable maybe putting back the atomic_index struct
member is the best solution after all.


  The spec says that the minimum an implementation should support is
 1, so it accomplishes that, but still... a quick look through the i965
 code reveals that it is prepared to support up to 16 for example (see
 BRW_MAX_ABO in brw_context.h).
 
 Iago
 
  V3: Dont make unrelated changes to the uniform storage handling.
  Cc the correct stable branch.
  
  V2: store buffer index in var-data.index and uniform slot in
  var-data.location to avoid issues when linking more than 2 shaders.
  Also some small tidy ups.
  
  Cc: Alejandro Piñeiro apinhe...@igalia.com
  Cc: Ian Romanick i...@freedesktop.org
  Cc: 10.6 mesa-sta...@lists.freedesktop.org
  Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90175
  ---
   src/glsl/ir.h  | 2 ++
   src/glsl/link_atomics.cpp  | 2 ++
   src/glsl/nir/glsl_to_nir.cpp   | 2 --
   src/glsl/nir/nir.h | 5 +++--
   src/glsl/nir/nir_lower_atomics.c   | 2 +-
   src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 2 +-
   6 files changed, 9 insertions(+), 6 deletions(-)
  
  diff --git a/src/glsl/ir.h b/src/glsl/ir.h
  index ede8caa..f77534c 100644
  --- a/src/glsl/ir.h
  +++ b/src/glsl/ir.h
  @@ -757,6 +757,8 @@ public:
  * \note
  * The GLSL spec only allows the values 0 or 1 for the index in \b 
  dual
  * source blending.
  +   *
  +   * For atomic counters this stores the atomic buffer index.
  */
 unsigned index:1;
   
  diff --git a/src/glsl/link_atomics.cpp b/src/glsl/link_atomics.cpp
  index 100d03c..5120564 100644
  --- a/src/glsl/link_atomics.cpp
  +++ b/src/glsl/link_atomics.cpp
  @@ -204,6 +204,8 @@ link_assign_atomic_counter_resources(struct gl_context 
  *ctx,
if (!var-data.explicit_binding)
   var-data.binding = i;
   
  + var-data.index = i;
  +
storage-atomic_buffer_index = i;
storage-offset = var-data.atomic.offset;
storage-array_stride = (var-type-is_array() ?
  diff --git a/src/glsl/nir/glsl_to_nir.cpp b/src/glsl/nir/glsl_to_nir.cpp
  index 77327b6..6cb23c0 100644
  --- a/src/glsl/nir/glsl_to_nir.cpp
  +++ b/src/glsl/nir/glsl_to_nir.cpp
  @@ -326,8 +326,6 @@ nir_visitor::visit(ir_variable *ir)
   
  var-data.index = ir-data.index;
  var-data.binding = ir-data.binding;
  -   /* XXX Get rid of buffer_index */
  -   var-data.atomic.buffer_index = ir-data.binding;
  var-data.atomic.offset = ir-data.atomic.offset;
  var-data.image.read_only = ir-data.image_read_only;
  var-data.image.write_only = ir-data.image_write_only;
  diff --git a/src/glsl/nir/nir.h b/src/glsl/nir/nir.h
  index 62cdbd4..837d197 100644
  --- a/src/glsl/nir/nir.h
  +++ b/src/glsl/nir/nir.h
  @@ -292,7 +292,9 @@ typedef struct {
 unsigned int driver_location;
   
 /**
  -   * output index for dual source blending.
  +   * Output index for dual source blending.
  +   *
  +   * For atomic counters this stores the atomic buffer index.
  */
 int index;
   
  @@ -307,7 +309,6 @@ typedef struct {
  * Location an atomic counter is stored at.
  */
 struct {
  - unsigned buffer_index;
unsigned offset;
 } atomic;
   
  diff --git a/src/glsl/nir/nir_lower_atomics.c 
  b/src/glsl/nir/nir_lower_atomics.c
  index ce3615a..6119f62 100644
  --- a/src/glsl/nir/nir_lower_atomics.c
  +++ b/src/glsl/nir/nir_lower_atomics.c
  @@ -63,7 +63,7 @@ lower_instr(nir_intrinsic_instr *instr, 
  nir_function_impl *impl)
   
  nir_intrinsic_instr *new_instr = nir_intrinsic_instr_create(mem_ctx, 
  op);
  new_instr-const_index[0] =
  -  (int) instr-variables[0]-var-data.atomic.buffer_index;
  +  (int) instr-variables[0]-var-data.index;
   
  nir_load_const_instr *offset_const = 
  nir_load_const_instr_create(mem_ctx, 1);
  offset_const-value.u[0] = instr-variables[0]-var
  -data.atomic.offset;
  

[Mesa-dev] [PATCH V3] glsl: fix atomic buffer index for bindings other than 0

[Timothy Arceri]

Since commit c0cd5b var-data.binding was being used as a replacement
for atomic buffer index, but they don't have to be the same value they
just happen to end up the same when binding is 0.

Now we store atomic buffer index in the unused var-data.index
to avoid the extra memory of putting back the atmoic buffer index field.

V3: Dont make unrelated changes to the uniform storage handling.
Cc the correct stable branch.

V2: store buffer index in var-data.index and uniform slot in
var-data.location to avoid issues when linking more than 2 shaders.
Also some small tidy ups.

Cc: Alejandro Piñeiro apinhe...@igalia.com
Cc: Ian Romanick i...@freedesktop.org
Cc: 10.6 mesa-sta...@lists.freedesktop.org
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90175
---
 src/glsl/ir.h  | 2 ++
 src/glsl/link_atomics.cpp  | 2 ++
 src/glsl/nir/glsl_to_nir.cpp   | 2 --
 src/glsl/nir/nir.h | 5 +++--
 src/glsl/nir/nir_lower_atomics.c   | 2 +-
 src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 2 +-
 6 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/src/glsl/ir.h b/src/glsl/ir.h
index ede8caa..f77534c 100644
--- a/src/glsl/ir.h
+++ b/src/glsl/ir.h
@@ -757,6 +757,8 @@ public:
* \note
* The GLSL spec only allows the values 0 or 1 for the index in \b dual
* source blending.
+   *
+   * For atomic counters this stores the atomic buffer index.
*/
   unsigned index:1;
 
diff --git a/src/glsl/link_atomics.cpp b/src/glsl/link_atomics.cpp
index 100d03c..5120564 100644
--- a/src/glsl/link_atomics.cpp
+++ b/src/glsl/link_atomics.cpp
@@ -204,6 +204,8 @@ link_assign_atomic_counter_resources(struct gl_context *ctx,
  if (!var-data.explicit_binding)
 var-data.binding = i;
 
+ var-data.index = i;
+
  storage-atomic_buffer_index = i;
  storage-offset = var-data.atomic.offset;
  storage-array_stride = (var-type-is_array() ?
diff --git a/src/glsl/nir/glsl_to_nir.cpp b/src/glsl/nir/glsl_to_nir.cpp
index 77327b6..6cb23c0 100644
--- a/src/glsl/nir/glsl_to_nir.cpp
+++ b/src/glsl/nir/glsl_to_nir.cpp
@@ -326,8 +326,6 @@ nir_visitor::visit(ir_variable *ir)
 
var-data.index = ir-data.index;
var-data.binding = ir-data.binding;
-   /* XXX Get rid of buffer_index */
-   var-data.atomic.buffer_index = ir-data.binding;
var-data.atomic.offset = ir-data.atomic.offset;
var-data.image.read_only = ir-data.image_read_only;
var-data.image.write_only = ir-data.image_write_only;
diff --git a/src/glsl/nir/nir.h b/src/glsl/nir/nir.h
index 62cdbd4..837d197 100644
--- a/src/glsl/nir/nir.h
+++ b/src/glsl/nir/nir.h
@@ -292,7 +292,9 @@ typedef struct {
   unsigned int driver_location;
 
   /**
-   * output index for dual source blending.
+   * Output index for dual source blending.
+   *
+   * For atomic counters this stores the atomic buffer index.
*/
   int index;
 
@@ -307,7 +309,6 @@ typedef struct {
* Location an atomic counter is stored at.
*/
   struct {
- unsigned buffer_index;
  unsigned offset;
   } atomic;
 
diff --git a/src/glsl/nir/nir_lower_atomics.c b/src/glsl/nir/nir_lower_atomics.c
index ce3615a..6119f62 100644
--- a/src/glsl/nir/nir_lower_atomics.c
+++ b/src/glsl/nir/nir_lower_atomics.c
@@ -63,7 +63,7 @@ lower_instr(nir_intrinsic_instr *instr, nir_function_impl 
*impl)
 
nir_intrinsic_instr *new_instr = nir_intrinsic_instr_create(mem_ctx, op);
new_instr-const_index[0] =
-  (int) instr-variables[0]-var-data.atomic.buffer_index;
+  (int) instr-variables[0]-var-data.index;
 
nir_load_const_instr *offset_const = nir_load_const_instr_create(mem_ctx, 
1);
offset_const-value.u[0] = instr-variables[0]-var-data.atomic.offset;
diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp 
b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
index 6fee798..5e0bf1b 100644
--- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
+++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
@@ -2407,7 +2407,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.binding);
+  location-data.index);
 
/* Calculate the surface offset */
src_reg offset(this, glsl_type::uint_type);
-- 
2.4.3

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev