[Mesa-dev] [PATCH] nir: split SSBO min/max atomic instrinsics into signed/unsigned versions
Ilia Mirkin
imirkin at alum.mit.edu
Mon Sep 28 13:59:21 PDT 2015
On Mon, Sep 28, 2015 at 4:55 PM, Kristian Høgsberg <krh at bitplanet.net> wrote:
> On Mon, Sep 28, 2015 at 1:47 AM, Iago Toral Quiroga <itoral at igalia.com> wrote:
>> NIR is typeless so this is the only way to keep track of the
>> type to select the proper atomic to use.
>> ---
>> I decided to squash the i965 changes in because otherwise we would break
>> the build between the nir and i965 patches. Let me know if we rather
>> split them anyway.
>>
>> src/glsl/nir/glsl_to_nir.cpp | 22 ++++++++++++++++++----
>> src/glsl/nir/nir_intrinsics.h | 6 ++++--
>> src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 20 ++++++++++----------
>> src/mesa/drivers/dri/i965/brw_vec4_nir.cpp | 22 +++++++++++-----------
>> 4 files changed, 43 insertions(+), 27 deletions(-)
>>
>> diff --git a/src/glsl/nir/glsl_to_nir.cpp b/src/glsl/nir/glsl_to_nir.cpp
>> index f03a107..27e9276 100644
>> --- a/src/glsl/nir/glsl_to_nir.cpp
>> +++ b/src/glsl/nir/glsl_to_nir.cpp
>> @@ -662,9 +662,21 @@ nir_visitor::visit(ir_call *ir)
>> } else if (strcmp(ir->callee_name(), "__intrinsic_ssbo_atomic_xor_internal") == 0) {
>> op = nir_intrinsic_ssbo_atomic_xor;
>> } else if (strcmp(ir->callee_name(), "__intrinsic_ssbo_atomic_min_internal") == 0) {
>> - op = nir_intrinsic_ssbo_atomic_min;
>> + assert(ir->return_deref);
>> + if (ir->return_deref->type == glsl_type::int_type)
>> + op = nir_intrinsic_ssbo_atomic_min_d;
>> + else if (ir->return_deref->type == glsl_type::uint_type)
>> + op = nir_intrinsic_ssbo_atomic_min_ud;
>> + else
>> + unreachable("Not reached");
>> } else if (strcmp(ir->callee_name(), "__intrinsic_ssbo_atomic_max_internal") == 0) {
>> - op = nir_intrinsic_ssbo_atomic_max;
>> + assert(ir->return_deref);
>> + if (ir->return_deref->type == glsl_type::int_type)
>> + op = nir_intrinsic_ssbo_atomic_max_d;
>> + else if (ir->return_deref->type == glsl_type::uint_type)
>> + op = nir_intrinsic_ssbo_atomic_max_ud;
>> + else
>> + unreachable("Not reached");
>> } else if (strcmp(ir->callee_name(), "__intrinsic_ssbo_atomic_exchange_internal") == 0) {
>> op = nir_intrinsic_ssbo_atomic_exchange;
>> } else if (strcmp(ir->callee_name(), "__intrinsic_ssbo_atomic_comp_swap_internal") == 0) {
>> @@ -874,8 +886,10 @@ nir_visitor::visit(ir_call *ir)
>> break;
>> }
>> case nir_intrinsic_ssbo_atomic_add:
>> - case nir_intrinsic_ssbo_atomic_min:
>> - case nir_intrinsic_ssbo_atomic_max:
>> + case nir_intrinsic_ssbo_atomic_min_d:
>> + case nir_intrinsic_ssbo_atomic_min_ud:
>> + case nir_intrinsic_ssbo_atomic_max_d:
>> + case nir_intrinsic_ssbo_atomic_max_ud:
>> case nir_intrinsic_ssbo_atomic_and:
>> case nir_intrinsic_ssbo_atomic_or:
>> case nir_intrinsic_ssbo_atomic_xor:
>> diff --git a/src/glsl/nir/nir_intrinsics.h b/src/glsl/nir/nir_intrinsics.h
>> index 06f1b02..956e68d 100644
>> --- a/src/glsl/nir/nir_intrinsics.h
>> +++ b/src/glsl/nir/nir_intrinsics.h
>> @@ -174,8 +174,10 @@ INTRINSIC(image_samples, 0, ARR(), true, 1, 1, 0,
>> * 3: For CompSwap only: the second data parameter.
>> */
>> INTRINSIC(ssbo_atomic_add, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
>> -INTRINSIC(ssbo_atomic_min, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
>> -INTRINSIC(ssbo_atomic_max, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
>> +INTRINSIC(ssbo_atomic_min_d, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
>> +INTRINSIC(ssbo_atomic_min_ud, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
>> +INTRINSIC(ssbo_atomic_max_d, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
>> +INTRINSIC(ssbo_atomic_max_ud, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
>> INTRINSIC(ssbo_atomic_and, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
>> INTRINSIC(ssbo_atomic_or, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
>> INTRINSIC(ssbo_atomic_xor, 3, ARR(1, 1, 1), true, 1, 0, 0, 0)
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>> index a2bc5c6..9fcddab 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
>> @@ -1870,17 +1870,17 @@ fs_visitor::nir_emit_intrinsic(const fs_builder &bld, nir_intrinsic_instr *instr
>> case nir_intrinsic_ssbo_atomic_add:
>> nir_emit_ssbo_atomic(bld, BRW_AOP_ADD, instr);
>> break;
>> - case nir_intrinsic_ssbo_atomic_min:
>> - if (dest.type == BRW_REGISTER_TYPE_D)
>> - nir_emit_ssbo_atomic(bld, BRW_AOP_IMIN, instr);
>> - else
>> - nir_emit_ssbo_atomic(bld, BRW_AOP_UMIN, instr);
>> + case nir_intrinsic_ssbo_atomic_min_d:
>> + nir_emit_ssbo_atomic(bld, BRW_AOP_IMIN, instr);
>> break;
>> - case nir_intrinsic_ssbo_atomic_max:
>> - if (dest.type == BRW_REGISTER_TYPE_D)
>> - nir_emit_ssbo_atomic(bld, BRW_AOP_IMAX, instr);
>> - else
>> - nir_emit_ssbo_atomic(bld, BRW_AOP_UMAX, instr);
>> + case nir_intrinsic_ssbo_atomic_min_ud:
>> + nir_emit_ssbo_atomic(bld, BRW_AOP_UMIN, instr);
>> + break;
>> + case nir_intrinsic_ssbo_atomic_max_d:
>> + nir_emit_ssbo_atomic(bld, BRW_AOP_IMAX, instr);
>> + break;
>> + case nir_intrinsic_ssbo_atomic_max_ud:
>> + nir_emit_ssbo_atomic(bld, BRW_AOP_UMAX, instr);
>> break;
>
> Francisco did something else for image atomics:
>
> /* Get the referenced image variable and type. */
> const nir_variable *var = instr->variables[0]->var;
> const glsl_type *type = var->type->without_array();
>
> and then determine the atomic operand based on the intrinsic and the
> type, like you did in your original approach. Just this time, the type
> is actually valid. I think that's a nicer approach and we should
> definitely do it the same way for images and SSBOs.
Not sure what your comment is saying, but in case you're advocating
for keeping the combined ops, I'd like to point out that for
tgsi_to_nir, we definitely need the independent intrinsics. TGSI is
entirely typeless and doesn't carry nearly the level of information
that NIR does (because it's simple and serializable). So there are
separate ATOMIMAX/ATOMUMAX tgsi opcodes, and ideally those need to be
convertable into separate nir intrinsics.
I guess we could fake some weird stuff with variables, but I'd really
rather not.
Cheers,
-ilia
More information about the mesa-dev
mailing list