[Mesa-dev] [PATCH] spirv: Fix a couple of image atomic load/store bugs
Jason Ekstrand
jason at jlekstrand.net
Mon Jul 16 04:30:25 UTC 2018
On Sun, Jul 15, 2018 at 3:11 AM Bas Nieuwenhuizen <bas at basnieuwenhuizen.nl>
wrote:
> On Sun, Jul 15, 2018 at 3:56 AM, Jason Ekstrand <jason at jlekstrand.net>
> wrote:
> > For one thing, the NIR opcodes for image load/store always take and
> > return a vec4 value regardless of the image type. We need to fix up
> > both the source and destination to handle it. For another thing, we
> > weren't actually setting up a destination in the OpAtomicLoad case.
>
> Nominate for stable? I actually encountered this in an application.
>
> > ---
> > src/compiler/spirv/spirv_to_nir.c | 44 +++++++++++++++++++------------
> > 1 file changed, 27 insertions(+), 17 deletions(-)
> >
> > diff --git a/src/compiler/spirv/spirv_to_nir.c
> b/src/compiler/spirv/spirv_to_nir.c
> > index c31293a77be..a204bfa39ba 100644
> > --- a/src/compiler/spirv/spirv_to_nir.c
> > +++ b/src/compiler/spirv/spirv_to_nir.c
> > @@ -2309,6 +2309,15 @@ get_image_coord(struct vtn_builder *b, uint32_t
> value)
> > return nir_swizzle(&b->nb, coord->def, swizzle, 4, false);
> > }
> >
> > +static nir_ssa_def *
> > +expand_to_vec4(nir_builder *b, nir_ssa_def *value)
> > +{
> > + unsigned swiz[4];
> > + for (unsigned i = 0; i < 4; i++)
> > + swiz[i] = i < value->num_components ? i : 0;
> > + return nir_swizzle(b, value, swiz, 4, false);
>
> Maybe skip if value already has 4 components?
>
Done.
> Either way,
>
Reviewed-by: Bas Nieuwenhuizen <bas at basnieuwenhuizen.nl>
>
Thanks! I'll push in the morning assuming my Jenkins run is happy.
--Uason
> > +}
> > +
> > static void
> > vtn_handle_image(struct vtn_builder *b, SpvOp opcode,
> > const uint32_t *w, unsigned count)
> > @@ -2422,11 +2431,7 @@ vtn_handle_image(struct vtn_builder *b, SpvOp
> opcode,
> > /* The image coordinate is always 4 components but we may not
> have that
> > * many. Swizzle to compensate.
> > */
> > - unsigned swiz[4];
> > - for (unsigned i = 0; i < 4; i++)
> > - swiz[i] = i < image.coord->num_components ? i : 0;
> > - intrin->src[1] = nir_src_for_ssa(nir_swizzle(&b->nb, image.coord,
> > - swiz, 4, false));
> > + intrin->src[1] = nir_src_for_ssa(expand_to_vec4(&b->nb,
> image.coord));
> > intrin->src[2] = nir_src_for_ssa(image.sample);
> > }
> >
> > @@ -2436,11 +2441,13 @@ vtn_handle_image(struct vtn_builder *b, SpvOp
> opcode,
> > case SpvOpImageRead:
> > break;
> > case SpvOpAtomicStore:
> > - intrin->src[3] = nir_src_for_ssa(vtn_ssa_value(b, w[4])->def);
> > - break;
> > - case SpvOpImageWrite:
> > - intrin->src[3] = nir_src_for_ssa(vtn_ssa_value(b, w[3])->def);
> > + case SpvOpImageWrite: {
> > + const uint32_t value_id = opcode == SpvOpAtomicStore ? w[4] :
> w[3];
> > + nir_ssa_def *value = vtn_ssa_value(b, value_id)->def;
> > + /* nir_intrinsic_image_deref_store always takes a vec4 value */
> > + intrin->src[3] = nir_src_for_ssa(expand_to_vec4(&b->nb, value));
> > break;
> > + }
> >
> > case SpvOpAtomicCompareExchange:
> > case SpvOpAtomicIIncrement:
> > @@ -2462,23 +2469,26 @@ vtn_handle_image(struct vtn_builder *b, SpvOp
> opcode,
> > vtn_fail("Invalid image opcode");
> > }
> >
> > - if (opcode != SpvOpImageWrite) {
> > + if (opcode != SpvOpImageWrite && opcode != SpvOpAtomicStore) {
> > struct vtn_value *val = vtn_push_value(b, w[2],
> vtn_value_type_ssa);
> > struct vtn_type *type = vtn_value(b, w[1],
> vtn_value_type_type)->type;
> >
> > - unsigned dest_components = nir_intrinsic_dest_components(intrin);
> > - if (intrin->intrinsic == nir_intrinsic_image_deref_size) {
> > - dest_components = intrin->num_components =
> > - glsl_get_vector_elements(type->type);
> > - }
> > + unsigned dest_components = glsl_get_vector_elements(type->type);
> > + intrin->num_components = nir_intrinsic_infos[op].dest_components;
> > + if (intrin->num_components == 0)
> > + intrin->num_components = dest_components;
> >
> > nir_ssa_dest_init(&intrin->instr, &intrin->dest,
> > - dest_components, 32, NULL);
> > + intrin->num_components, 32, NULL);
> >
> > nir_builder_instr_insert(&b->nb, &intrin->instr);
> >
> > + nir_ssa_def *result = &intrin->dest.ssa;
> > + if (intrin->num_components != dest_components)
> > + result = nir_channels(&b->nb, result, (1 << dest_components) -
> 1);
> > +
> > val->ssa = vtn_create_ssa_value(b, type->type);
> > - val->ssa->def = &intrin->dest.ssa;
> > + val->ssa->def = result;
> > } else {
> > nir_builder_instr_insert(&b->nb, &intrin->instr);
> > }
> > --
> > 2.17.1
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180715/d93bd0c4/attachment.html>
More information about the mesa-dev
mailing list