Kristian Høgsberg <[email protected]> writes: > On Mon, Sep 28, 2015 at 1:59 PM, Ilia Mirkin <[email protected]> wrote: >> On Mon, Sep 28, 2015 at 4:55 PM, Kristian Høgsberg <[email protected]> >> wrote: >>> On Mon, Sep 28, 2015 at 1:47 AM, Iago Toral Quiroga <[email protected]> >>> 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 > [email protected] > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/mesa-dev
