[Mesa-dev] [PATCH] spirv/nir: Add support for OpAtomicLoad/Store
Timothy Arceri
timothy.arceri at collabora.com
Sun Sep 11 23:16:20 UTC 2016
On Wed, 2016-09-07 at 19:28 +0100, Lionel Landwerlin wrote:
> Hi Mark,
>
> Thanks for the report.
> The assumption is that coord will be set on the image by a preceding
> SpvOpImageTexelPointer opcode (see top of the
> vtn_handle_image function).
I don't see how that can be right.
The image struct is declared on line 1663 after the handling of
the SpvOpImageTexelPointer opcode.
struct vtn_image_pointer image;
You then set image.image for the Atomic op codes. Neither image.coord
or image.sample are set for this code path.
As well as the Coverity issue there is also a GCC warning:
In file included from spirv/vtn_private.h:28:0,
from spirv/spirv_to_nir.c:28:
spirv/spirv_to_nir.c: In function ‘vtn_handle_image’:
./nir/nir.h:571:11: warning: ‘image.coord’ may be used uninitialized in
this function [-Wmaybe-uninitialized]
return src;
^~~
spirv/spirv_to_nir.c:1663:29: note: ‘image.coord’ was declared here
struct vtn_image_pointer image;
^~~~~
spirv/spirv_to_nir.c:1776:22: warning: ‘image.sample’ may be used
uninitialized in this function [-Wmaybe-uninitialized]
intrin->src[1] = nir_src_for_ssa(image.sample);
>
> I don't think there are tests in the CTS exercising this at the
> moment
> :(
>
> -
> Lionel
>
> On Wed, 2016-09-07 at 10:14 -0700, Mark Janes wrote:
> >
> > Hi Lionel,
> >
> > This patch triggered CID 1372521 in Coverity. Please let me know
> > if
> > my
> > the analysis is correct below:
> >
> > Lionel Landwerlin <llandwerlin at gmail.com> writes:
> >
> > >
> > >
> > > Fixes new CTS tests :
> > >
> > > dEQP-VK.spirv_assembly.instruction.compute.opatomic.load
> > > dEQP-VK.spirv_assembly.instruction.compute.opatomic.store
> > >
> > > v2: don't handle images like ssbo/ubo (Jason)
> > >
> > > Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
> > > Cc: Jason Ekstrand <jason.ekstrand at intel.com>
> > > ---
> > > src/compiler/spirv/spirv_to_nir.c | 124
> > > ++++++++++++++++++++++++++++++++++----
> > > 1 file changed, 113 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/src/compiler/spirv/spirv_to_nir.c
> > > b/src/compiler/spirv/spirv_to_nir.c
> > > index fda38f9..1fcd70f 100644
> > > --- a/src/compiler/spirv/spirv_to_nir.c
> > > +++ b/src/compiler/spirv/spirv_to_nir.c
> > > @@ -1640,6 +1640,18 @@ vtn_handle_image(struct vtn_builder *b,
> > > SpvOp opcode,
> > > image = *vtn_value(b, w[3], vtn_value_type_image_pointer)-
> > > >
> > > > image;
> > > break;
> > >
> > > + case SpvOpAtomicLoad: {
> > > + image.image =
> > > + vtn_value(b, w[3], vtn_value_type_access_chain)-
> > > >
> > > > access_chain;
> > > + break;
> > > + }
> > > +
> > > + case SpvOpAtomicStore: {
> > > + image.image =
> > > + vtn_value(b, w[1], vtn_value_type_access_chain)-
> > > >
> > > > access_chain;
> > > + break;
> > > + }
> >
> > These cases do not initialize image.coord, which is dereferenced on
> > line
> > 1773. SpvOpImageQuerySize is a similar case which sets image.coord
> > to
> > NULL and is excepted from the code path at line 1773.
> >
> > >
> > >
> > > case SpvOpImageQuerySize:
> > > image.image =
> > > vtn_value(b, w[3], vtn_value_type_access_chain)-
> > > >
> > > > access_chain;
> > > @@ -1685,6 +1697,8 @@ vtn_handle_image(struct vtn_builder *b,
> > > SpvOp
> > > opcode,
> > > OP(ImageQuerySize, size)
> > > OP(ImageRead, load)
> > > OP(ImageWrite, store)
> > > + OP(AtomicLoad, load)
> > > + OP(AtomicStore, store)
> > > OP(AtomicExchange, atomic_exchange)
> > > OP(AtomicCompareExchange, atomic_comp_swap)
> > > OP(AtomicIIncrement, atomic_add)
> > > @@ -1723,9 +1737,13 @@ vtn_handle_image(struct vtn_builder *b,
> > > SpvOp opcode,
> > > }
> > >
> > > switch (opcode) {
> > > + case SpvOpAtomicLoad:
> > > case SpvOpImageQuerySize:
> > > case SpvOpImageRead:
> > > break;
> > > + case SpvOpAtomicStore:
> > > + intrin->src[2] = nir_src_for_ssa(vtn_ssa_value(b, w[4])-
> > > >
> > > > def);
> > > + break;
> > > case SpvOpImageWrite:
> > > intrin->src[2] = nir_src_for_ssa(vtn_ssa_value(b, w[3])-
> > > >
> > > > def);
> > > break;
> > > @@ -1784,6 +1802,8 @@ static nir_intrinsic_op
> > > get_ssbo_nir_atomic_op(SpvOp opcode)
> > > {
> > > switch (opcode) {
> > > + case SpvOpAtomicLoad: return nir_intrinsic_load_ssbo;
> > > + case SpvOpAtomicStore: return nir_intrinsic_store_ssbo;
> > > #define OP(S, N) case SpvOp##S: return nir_intrinsic_ssbo_##N;
> > > OP(AtomicExchange, atomic_exchange)
> > > OP(AtomicCompareExchange, atomic_comp_swap)
> > > @@ -1808,6 +1828,8 @@ static nir_intrinsic_op
> > > get_shared_nir_atomic_op(SpvOp opcode)
> > > {
> > > switch (opcode) {
> > > + case SpvOpAtomicLoad: return nir_intrinsic_load_var;
> > > + case SpvOpAtomicStore: return nir_intrinsic_store_var;
> > > #define OP(S, N) case SpvOp##S: return nir_intrinsic_var_##N;
> > > OP(AtomicExchange, atomic_exchange)
> > > OP(AtomicCompareExchange, atomic_comp_swap)
> > > @@ -1873,10 +1895,38 @@ static void
> > > vtn_handle_ssbo_or_shared_atomic(struct vtn_builder *b, SpvOp
> > > opcode,
> > > const uint32_t *w, unsigned
> > > count)
> > > {
> > > - struct vtn_access_chain *chain =
> > > - vtn_value(b, w[3], vtn_value_type_access_chain)-
> > > >
> > > > access_chain;
> > > + struct vtn_access_chain *chain;
> > > nir_intrinsic_instr *atomic;
> > >
> > > + switch (opcode) {
> > > + case SpvOpAtomicLoad:
> > > + case SpvOpAtomicExchange:
> > > + case SpvOpAtomicCompareExchange:
> > > + case SpvOpAtomicCompareExchangeWeak:
> > > + case SpvOpAtomicIIncrement:
> > > + case SpvOpAtomicIDecrement:
> > > + case SpvOpAtomicIAdd:
> > > + case SpvOpAtomicISub:
> > > + case SpvOpAtomicSMin:
> > > + case SpvOpAtomicUMin:
> > > + case SpvOpAtomicSMax:
> > > + case SpvOpAtomicUMax:
> > > + case SpvOpAtomicAnd:
> > > + case SpvOpAtomicOr:
> > > + case SpvOpAtomicXor:
> > > + chain =
> > > + vtn_value(b, w[3], vtn_value_type_access_chain)-
> > > >
> > > > access_chain;
> > > + break;
> > > +
> > > + case SpvOpAtomicStore:
> > > + chain =
> > > + vtn_value(b, w[1], vtn_value_type_access_chain)-
> > > >
> > > > access_chain;
> > > + break;
> > > +
> > > + default:
> > > + unreachable("Invalid SPIR-V atomic");
> > > + }
> > > +
> > > /*
> > > SpvScope scope = w[4];
> > > SpvMemorySemanticsMask semantics = w[5];
> > > @@ -1897,18 +1947,58 @@ vtn_handle_ssbo_or_shared_atomic(struct
> > > vtn_builder *b, SpvOp opcode,
> > > nir_intrinsic_op op = get_ssbo_nir_atomic_op(opcode);
> > >
> > > atomic = nir_intrinsic_instr_create(b->nb.shader, op);
> > > - atomic->src[0] = nir_src_for_ssa(index);
> > > - atomic->src[1] = nir_src_for_ssa(offset);
> > > - fill_common_atomic_sources(b, opcode, w, &atomic->src[2]);
> > > +
> > > + switch (opcode) {
> > > + case SpvOpAtomicLoad:
> > > + atomic->num_components = glsl_get_vector_elements(type-
> > > >
> > > > type);
> > > + atomic->src[0] = nir_src_for_ssa(index);
> > > + atomic->src[1] = nir_src_for_ssa(offset);
> > > + break;
> > > +
> > > + case SpvOpAtomicStore:
> > > + atomic->num_components = glsl_get_vector_elements(type-
> > > >
> > > > type);
> > > + nir_intrinsic_set_write_mask(atomic, (1 << atomic-
> > > >
> > > > num_components) - 1);
> > > + atomic->src[0] = nir_src_for_ssa(vtn_ssa_value(b,
> > > w[4])-
> > > >
> > > > def);
> > > + atomic->src[1] = nir_src_for_ssa(index);
> > > + atomic->src[2] = nir_src_for_ssa(offset);
> > > + break;
> > > +
> > > + case SpvOpAtomicExchange:
> > > + case SpvOpAtomicCompareExchange:
> > > + case SpvOpAtomicCompareExchangeWeak:
> > > + case SpvOpAtomicIIncrement:
> > > + case SpvOpAtomicIDecrement:
> > > + case SpvOpAtomicIAdd:
> > > + case SpvOpAtomicISub:
> > > + case SpvOpAtomicSMin:
> > > + case SpvOpAtomicUMin:
> > > + case SpvOpAtomicSMax:
> > > + case SpvOpAtomicUMax:
> > > + case SpvOpAtomicAnd:
> > > + case SpvOpAtomicOr:
> > > + case SpvOpAtomicXor:
> > > + atomic->src[0] = nir_src_for_ssa(index);
> > > + atomic->src[1] = nir_src_for_ssa(offset);
> > > + fill_common_atomic_sources(b, opcode, w, &atomic-
> > > >
> > > > src[2]);
> > > + break;
> > > +
> > > + default:
> > > + unreachable("Invalid SPIR-V atomic");
> > > + }
> > > }
> > >
> > > - nir_ssa_dest_init(&atomic->instr, &atomic->dest, 1, 32,
> > > NULL);
> > > + if (opcode != SpvOpAtomicStore) {
> > > + struct vtn_type *type = vtn_value(b, w[1],
> > > vtn_value_type_type)->type;
> > > +
> > > + nir_ssa_dest_init(&atomic->instr, &atomic->dest,
> > > + glsl_get_vector_elements(type->type),
> > > + glsl_get_bit_size(type->type), NULL);
> > >
> > > - struct vtn_type *type = vtn_value(b, w[1],
> > > vtn_value_type_type)->type;
> > > - struct vtn_value *val = vtn_push_value(b, w[2],
> > > vtn_value_type_ssa);
> > > - val->ssa = rzalloc(b, struct vtn_ssa_value);
> > > - val->ssa->def = &atomic->dest.ssa;
> > > - val->ssa->type = type->type;
> > > + struct vtn_value *val = vtn_push_value(b, w[2],
> > > vtn_value_type_ssa);
> > > + val->ssa = rzalloc(b, struct vtn_ssa_value);
> > > + val->ssa->def = &atomic->dest.ssa;
> > > + val->ssa->type = type->type;
> > > + }
> > >
> > > nir_builder_instr_insert(&b->nb, &atomic->instr);
> > > }
> > > @@ -2690,6 +2780,7 @@ vtn_handle_body_instruction(struct
> > > vtn_builder *b, SpvOp opcode,
> > > break;
> > > }
> > >
> > > + case SpvOpAtomicLoad:
> > > case SpvOpAtomicExchange:
> > > case SpvOpAtomicCompareExchange:
> > > case SpvOpAtomicCompareExchangeWeak:
> > > @@ -2714,6 +2805,17 @@ vtn_handle_body_instruction(struct
> > > vtn_builder *b, SpvOp opcode,
> > > break;
> > > }
> > >
> > > + case SpvOpAtomicStore: {
> > > + struct vtn_value *pointer = vtn_untyped_value(b, w[1]);
> > > + if (pointer->value_type == vtn_value_type_image_pointer) {
> > > + vtn_handle_image(b, opcode, w, count);
> > > + } else {
> > > + assert(pointer->value_type ==
> > > vtn_value_type_access_chain);
> > > + vtn_handle_ssbo_or_shared_atomic(b, opcode, w, count);
> > > + }
> > > + break;
> > > + }
> > > +
> > > case SpvOpSNegate:
> > > case SpvOpFNegate:
> > > case SpvOpNot:
> > > --
> > > 2.9.3
> > >
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list