[Mesa-dev] [PATCH] nir: split SSBO min/max atomic instrinsics into signed/unsigned versions
Francisco Jerez
currojerez at riseup.net
Tue Sep 29 06:42:29 PDT 2015
Kristian Høgsberg <krh at bitplanet.net> writes:
> 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.
>
Yeah, the signed and unsigned versions of the image atomic ops would be
redundant because image locations are always typed even in NIR, in fact
image variables carry texel format meta-data which is necessary in
certain cases for the back-end to be able to do the right form of texel
packing and unpacking. OTOH SSBOs are lowered to an untyped buffer
index/offset pair so the type information is lost and the separate
U/I intrinsics are justified IMO.
> 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
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150929/3a55a780/attachment.sig>
More information about the mesa-dev
mailing list