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
