[Mesa-dev] [PATCH 1/3] spirv: fix OpBitcast when the src and dst bitsize are different

[Jason Ekstrand]

On Thu, Jun 8, 2017 at 3:05 PM, Connor Abbott <connora at valvesoftware.com>
wrote:

> From: Connor Abbott <cwabbott0 at gmail.com>
>
> Before, we were just implementing it with a move, which is incorrect
> when the source and destination have different bitsizes. To implement
> it properly, we need to use the 64-bit pack/unpack opcodes. Since
> glslang uses OpBitcast to implement packInt2x32 and unpackInt2x32, this
> should fix them on anv (and radv once we enable the int64 capability).
>
> Fixes: b3135c3c ("anv: Advertise shaderInt64 on Broadwell and above")
> Signed-off-by: Connor Abbott <cwabbott0 at gmail.com>
> ---
>  src/compiler/spirv/vtn_alu.c | 68 ++++++++++++++++++++++++++++++
> +++++++++++++-
>  1 file changed, 67 insertions(+), 1 deletion(-)
>
> diff --git a/src/compiler/spirv/vtn_alu.c b/src/compiler/spirv/vtn_alu.c
> index 9e4beed..cd9569b 100644
> --- a/src/compiler/spirv/vtn_alu.c
> +++ b/src/compiler/spirv/vtn_alu.c
> @@ -210,6 +210,69 @@ vtn_handle_matrix_alu(struct vtn_builder *b, SpvOp
> opcode,
>     }
>  }
>
> +static void
> +vtn_handle_bitcast(struct vtn_builder *b, struct vtn_ssa_value *dest,
> +                   struct nir_ssa_def *src)
> +{
> +   if (glsl_get_vector_elements(dest->type) == src->num_components) {
> +      /* From the definition of OpBitcast in the SPIR-V 1.2 spec:
> +       *
> +       * "If Result Type has the same number of components as Operand,
> they
> +       * must also have the same component width, and results are
> computed per
> +       * component."
> +       */
> +      dest->def = nir_imov(&b->nb, src);
> +      return;
> +   }
> +
> +   /* From the definition of OpBitcast in the SPIR-V 1.2 spec:
> +    *
> +    * "If Result Type has a different number of components than Operand,
> the
> +    * total number of bits in Result Type must equal the total number of
> bits
> +    * in Operand. Let L be the type, either Result Type or Operand’s
> type, that
> +    * has the larger number of components. Let S be the other type, with
> the
> +    * smaller number of components. The number of components in L must be
> an
> +    * integer multiple of the number of components in S. The first
> component
> +    * (that is, the only or lowest-numbered component) of S maps to the
> first
> +    * components of L, and so on, up to the last component of S mapping
> to the
> +    * last components of L. Within this mapping, any single component of S
> +    * (mapping to multiple components of L) maps its lower-ordered bits
> to the
> +    * lower-numbered components of L."
> +    *
> +    * Since booleans are a separate type without a width, presumably they
> can't
> +    * be bitcasted. So we only have to deal with 32 vs. 64 bit right now,
> which
> +    * we handle using the pack/unpack opcodes.
>

This won't last long.... The Igalia guys already have patches in the works
to add 16-bit support.  Also, I'm guessing 8-bit ints are going to be a
thing at some point.  I think we can make this more general and make their
lives easier...


> +    */
> +   unsigned src_bit_size = src->bit_size;
> +   unsigned dest_bit_size = glsl_get_bit_size(dest->type);
> +   unsigned src_components = src->num_components;
> +   unsigned dest_components = glsl_get_vector_elements(dest->type);
>

How about something like this:

unsigned size = src_components * src_bit_size;
assert(size == dest_components * dest_bit_size);
unsigned min_bit_size = MIN2(src_bit_size, dest_bit_size);
unsigned total_comps = size / min_bit_size;

NIR_VLA(nir_ssa_def *, unpacked);

for (unsigned idx = 0, i = 0; i < src_comps; i++) {
   nir_ssa_def *chan = nir_channel(&b->nb, src, i);
   if (src_bit_size == min_bit_size) {
      total_comps[idx++] = chan;
   } else {
      assert(min_bit_size == 32 && src_bit_size == 64);
      nir_ssa_def *split =
         nir_unpack_64_2x32(&b->nb, nir_channel(&b->nb, src, comp));
      unpacked[idx++] = nir_channel(&b->nb, split, 0);
      unpacked[idx++] = nir_channel(&b->nb, split, 1);
   }
}

nir_ssa_def *vec_src[4];
for (unsigned idx = 0, i = 0; i < dest_comps; i++) {
   if (dest_bit_size == min_bit_size) {
      vec_src[i] = unpacked[idx++];
   } else {
      assert(min_bit_size == 32 && dest_bit_size == 64);
      nir_ssa_def *src0 = unpacked[idx++];
      nir_ssa_def *src1 = unpacked[idx++];
      vec_src[i] = nir_pack_64_2x32(&b->nb, nir_vec2(&b->nb, src0, src1));
   }
}

dest->def = nir_vec(&b->nb, vec_src, dest_components);

What do you think?  Another thought: Should this go in nir_builder?

--Jason


> +   if (src_bit_size > dest_bit_size) {
> +      assert(src_bit_size == 64);
> +      assert(dest_bit_size == 32);
> +      assert(dest_components == 2 * src_components);
> +      nir_ssa_def *dest_chan[4];
> +      for (unsigned comp = 0; comp < src_components; comp++) {
> +         nir_ssa_def *split =
> +            nir_unpack_64_2x32(&b->nb, nir_channel(&b->nb, src, comp));
>
+         dest_chan[2 * comp + 0] = nir_channel(&b->nb, split, 0);
> +         dest_chan[2 * comp + 1] = nir_channel(&b->nb, split, 1);
> +      }
> +      dest->def = nir_vec(&b->nb, dest_chan, dest_components);
> +   } else {
> +      assert(dest_bit_size == 64);
> +      assert(src_bit_size == 32);
> +      assert(src_components == 2 * dest_components);
> +      nir_ssa_def *dest_chan[4];
> +      for (unsigned comp = 0; comp < dest_components; comp++) {
> +         dest_chan[comp] =
> +            nir_pack_64_2x32(&b->nb, nir_channels(&b->nb, src,
> +                                                  0x3 << (2 * comp)));
> +      }
> +      dest->def = nir_vec(&b->nb, dest_chan, dest_components);
> +   }
> +}
> +
>  nir_op
>  vtn_nir_alu_op_for_spirv_opcode(SpvOp opcode, bool *swap,
>                                  nir_alu_type src, nir_alu_type dst)
> @@ -285,7 +348,6 @@ vtn_nir_alu_op_for_spirv_opcode(SpvOp opcode, bool
> *swap,
>     case SpvOpFUnordGreaterThanEqual:               return nir_op_fge;
>
>     /* Conversions: */
> -   case SpvOpBitcast:               return nir_op_imov;
>     case SpvOpQuantizeToF16:         return nir_op_fquantize2f16;
>     case SpvOpUConvert:
>     case SpvOpConvertFToU:
> @@ -503,6 +565,10 @@ vtn_handle_alu(struct vtn_builder *b, SpvOp opcode,
>        break;
>     }
>
> +   case SpvOpBitcast:
> +      vtn_handle_bitcast(b, val->ssa, src[0]);
> +      break;
> +
>     default: {
>        bool swap;
>        nir_alu_type src_alu_type = nir_get_nir_type_for_glsl_
> type(vtn_src[0]->type);
> --
> 2.9.4
>
> _______________________________________________
> 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/20170629/3256d47c/attachment-0001.html>