On Mon, Sep 28, 2015 at 1:59 PM, Ilia Mirkin <imir...@alum.mit.edu> wrote:
> On Mon, Sep 28, 2015 at 4:55 PM, Kristian Høgsberg <k...@bitplanet.net> wrote:
>> On Mon, Sep 28, 2015 at 1:47 AM, Iago Toral Quiroga <ito...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
