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 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 <llandwer...@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.landwer...@intel.com>
> > Cc: Jason Ekstrand <jason.ekstr...@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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
