[Mesa-dev] [PATCH] nir: split SSBO min/max atomic instrinsics into signed/unsigned versions
Kristian Høgsberg
krh at bitplanet.net
Mon Sep 28 15:07:14 PDT 2015
On Mon, Sep 28, 2015 at 1:59 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> 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,
I was saying that we do something else for image atomics and that we
should be consistent. Looking closer, I don't think we have the type
info in the SSBO case and Iago's path does the only right thing. We
could make signed and unsigned versions of the image atomics for
consistency (and maybe you'll need that for TGSI), but that's another
discussion.
Kristian
> 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