[Mesa-dev] [PATCH 15/18] nir: Fix OpAtomicCounterIDecrement for uniform atomic counters

[Timothy Arceri]

On 30/06/18 00:29, Alejandro Piñeiro wrote:
> From: Antia Puentes <apuentes at igalia.com>
> 
>  From the SPIR-V specification, OpAtomicIDecrement:
> "The instruction's result is the Original Value."
> 
> However, we were implementing it, for uniform atomic counters, as a
> pre-decrement operation.
> 
> Renamed the former nir intrinsic 'atomic_counter_dec*' to
> 'atomic_counter_pre_dec*' for clarification purposes, as it implements
> a pre-decrement operation as specified for OpenGL.
> 
> Added a new nir intrinsic 'atomic_counter_pos_dec*' which implements a
> post-decrement operation as required by SPIR-V.

Can we have proper OpenGL and spriv quotes here please?

Also if we must have two intrinsics can we use the name:

atomic_counter_post_dec_deref

pos tends to be used as an abreviation for position.


> ---
>   src/compiler/glsl/gl_nir_lower_atomics.c     | 8 ++++++--
>   src/compiler/glsl/glsl_to_nir.cpp            | 4 ++--
>   src/compiler/nir/nir_intrinsics.py           | 3 ++-
>   src/compiler/nir/nir_lower_atomics_to_ssbo.c | 8 +++++---
>   src/compiler/spirv/spirv_to_nir.c            | 2 +-
>   5 files changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/src/compiler/glsl/gl_nir_lower_atomics.c b/src/compiler/glsl/gl_nir_lower_atomics.c
> index 293730966fd..73d058f3902 100644
> --- a/src/compiler/glsl/gl_nir_lower_atomics.c
> +++ b/src/compiler/glsl/gl_nir_lower_atomics.c
> @@ -53,8 +53,12 @@ lower_deref_instr(nir_builder *b, nir_intrinsic_instr *instr,
>         op = nir_intrinsic_atomic_counter_inc;
>         break;
>   
> -   case nir_intrinsic_atomic_counter_dec_deref:
> -      op = nir_intrinsic_atomic_counter_dec;
> +   case nir_intrinsic_atomic_counter_pre_dec_deref:
> +      op = nir_intrinsic_atomic_counter_pre_dec;
> +      break;
> +
> +   case nir_intrinsic_atomic_counter_pos_dec_deref:
> +      op = nir_intrinsic_atomic_counter_pos_dec;
>         break;
>   
>      case nir_intrinsic_atomic_counter_add_deref:
> diff --git a/src/compiler/glsl/glsl_to_nir.cpp b/src/compiler/glsl/glsl_to_nir.cpp
> index d3a3fb9b085..2d76c7e6cfe 100644
> --- a/src/compiler/glsl/glsl_to_nir.cpp
> +++ b/src/compiler/glsl/glsl_to_nir.cpp
> @@ -630,7 +630,7 @@ nir_visitor::visit(ir_call *ir)
>            op = nir_intrinsic_atomic_counter_inc_deref;
>            break;
>         case ir_intrinsic_atomic_counter_predecrement:
> -         op = nir_intrinsic_atomic_counter_dec_deref;
> +         op = nir_intrinsic_atomic_counter_pre_dec_deref;
>            break;
>         case ir_intrinsic_atomic_counter_add:
>            op = nir_intrinsic_atomic_counter_add_deref;
> @@ -831,7 +831,7 @@ nir_visitor::visit(ir_call *ir)
>         switch (op) {
>         case nir_intrinsic_atomic_counter_read_deref:
>         case nir_intrinsic_atomic_counter_inc_deref:
> -      case nir_intrinsic_atomic_counter_dec_deref:
> +      case nir_intrinsic_atomic_counter_pre_dec_deref:
>         case nir_intrinsic_atomic_counter_add_deref:
>         case nir_intrinsic_atomic_counter_min_deref:
>         case nir_intrinsic_atomic_counter_max_deref:
> diff --git a/src/compiler/nir/nir_intrinsics.py b/src/compiler/nir/nir_intrinsics.py
> index d9d0bbdfccf..c5cbc09c598 100644
> --- a/src/compiler/nir/nir_intrinsics.py
> +++ b/src/compiler/nir/nir_intrinsics.py
> @@ -270,7 +270,8 @@ def atomic3(name):
>       intrinsic(name, src_comp=[1, 1, 1], dest_comp=1, indices=[BASE])
>   
>   atomic("atomic_counter_inc")
> -atomic("atomic_counter_dec")
> +atomic("atomic_counter_pre_dec")
> +atomic("atomic_counter_pos_dec")
>   atomic("atomic_counter_read", flags=[CAN_ELIMINATE])
>   atomic2("atomic_counter_add")
>   atomic2("atomic_counter_min")
> diff --git a/src/compiler/nir/nir_lower_atomics_to_ssbo.c b/src/compiler/nir/nir_lower_atomics_to_ssbo.c
> index 934ae81d750..34ac9fce40a 100644
> --- a/src/compiler/nir/nir_lower_atomics_to_ssbo.c
> +++ b/src/compiler/nir/nir_lower_atomics_to_ssbo.c
> @@ -71,7 +71,8 @@ lower_instr(nir_intrinsic_instr *instr, unsigned ssbo_offset, nir_builder *b)
>         return true;
>      case nir_intrinsic_atomic_counter_inc:
>      case nir_intrinsic_atomic_counter_add:
> -   case nir_intrinsic_atomic_counter_dec:
> +   case nir_intrinsic_atomic_counter_pre_dec:
> +   case nir_intrinsic_atomic_counter_pos_dec:
>         /* inc and dec get remapped to add: */
>         op = nir_intrinsic_ssbo_atomic_add;
>         break;
> @@ -119,7 +120,8 @@ lower_instr(nir_intrinsic_instr *instr, unsigned ssbo_offset, nir_builder *b)
>         nir_src_copy(&new_instr->src[1], &instr->src[0], new_instr);
>         new_instr->src[2] = nir_src_for_ssa(temp);
>         break;
> -   case nir_intrinsic_atomic_counter_dec:
> +   case nir_intrinsic_atomic_counter_pre_dec:
> +   case nir_intrinsic_atomic_counter_pos_dec:
>         /* remapped to ssbo_atomic_add: { buffer_idx, offset, -1 } */
>         /* NOTE semantic difference so we adjust the return value below */
>         temp = nir_imm_int(b, -1);
> @@ -148,7 +150,7 @@ lower_instr(nir_intrinsic_instr *instr, unsigned ssbo_offset, nir_builder *b)
>      nir_instr_insert_before(&instr->instr, &new_instr->instr);
>      nir_instr_remove(&instr->instr);
>   
> -   if (instr->intrinsic == nir_intrinsic_atomic_counter_dec) {
> +   if (instr->intrinsic == nir_intrinsic_atomic_counter_pre_dec) {
>         b->cursor = nir_after_instr(&new_instr->instr);
>         nir_ssa_def *result = nir_iadd(b, &new_instr->dest.ssa, temp);
>         nir_ssa_def_rewrite_uses(&instr->dest.ssa, nir_src_for_ssa(result));
> diff --git a/src/compiler/spirv/spirv_to_nir.c b/src/compiler/spirv/spirv_to_nir.c
> index 9d2f57cef94..b1ec0b892cb 100644
> --- a/src/compiler/spirv/spirv_to_nir.c
> +++ b/src/compiler/spirv/spirv_to_nir.c
> @@ -2519,7 +2519,7 @@ get_uniform_nir_atomic_op(struct vtn_builder *b, SpvOp opcode)
>      OP(AtomicExchange,         exchange)
>      OP(AtomicCompareExchange,  comp_swap)
>      OP(AtomicIIncrement,       inc_deref)
> -   OP(AtomicIDecrement,       dec_deref)
> +   OP(AtomicIDecrement,       pos_dec_deref)
>      OP(AtomicIAdd,             add_deref)
>      OP(AtomicISub,             add_deref)
>      OP(AtomicUMin,             min_deref)
>