[Mesa-dev] [PATCH 2/2] spirv: convert the offset and count operands for bitfield ops to uint32

[Samuel Iglesias Gonsálvez]

On 26/04/18 18:14, Jason Ekstrand wrote:
>
>
> On Thu, Apr 26, 2018 at 2:24 AM, Samuel Iglesias Gonsálvez
> <siglesias at igalia.com <mailto:siglesias at igalia.com>> wrote:
>
>     SPIR-V allows to define the shift operand for shift opcodes with
>     a bit-size different than 32 bits, but in NIR the opcodes have
>     that limitation. As agreed in the mailing list, this patch adds
>     a conversion to 32 bits to fix this.
>
>     For more info, see:
>
>     https://lists.freedesktop.org/archives/mesa-dev/2018-April/193026.html
>     <https://lists.freedesktop.org/archives/mesa-dev/2018-April/193026.html>
>
>     Signed-off-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com
>     <mailto:siglesias at igalia.com>>
>     ---
>      src/compiler/spirv/vtn_alu.c | 48
>     ++++++++++++++++++++++++++++++++++++++++++++
>      1 file changed, 48 insertions(+)
>
>     diff --git a/src/compiler/spirv/vtn_alu.c
>     b/src/compiler/spirv/vtn_alu.c
>     index 6f3b82cd5c3..a1654b20273 100644
>     --- a/src/compiler/spirv/vtn_alu.c
>     +++ b/src/compiler/spirv/vtn_alu.c
>     @@ -640,6 +640,54 @@ vtn_handle_alu(struct vtn_builder *b, SpvOp
>     opcode,
>            break;
>         }
>
>     +   case SpvOpBitFieldInsert: {
>     +      if (src[2]->bit_size != 32) {
>     +         /* Convert the Shift operand to 32 bits, which is the
>     bitsize
>     +          * supported by the NIR instruction. See discussion here:
>     +          *
>     +          *
>     https://lists.freedesktop.org/archives/mesa-dev/2018-April/193026.html
>     <https://lists.freedesktop.org/archives/mesa-dev/2018-April/193026.html>
>     +          */
>     +         src[2] = nir_build_alu(&b->nb, nir_op_u2u32, src[2],
>     NULL, NULL, NULL);
>     +      }
>     +      if (src[3]->bit_size != 32) {
>     +         /* Convert the Shift operand to 32 bits, which is the
>     bitsize
>     +          * supported by the NIR instruction. See discussion here:
>     +          *
>     +          *
>     https://lists.freedesktop.org/archives/mesa-dev/2018-April/193026.html
>     <https://lists.freedesktop.org/archives/mesa-dev/2018-April/193026.html>
>     +          */
>     +         src[3] = nir_build_alu(&b->nb, nir_op_u2u32, src[3],
>     NULL, NULL, NULL);
>     +      }
>     +      val->ssa->def = nir_build_alu(&b->nb,
>     nir_op_bitfield_insert, src[0], src[1], src[2], src[3]);
>
>
> Just use nir_bitfield_insert here
>  
>
>     +      break;
>     +   }
>     +
>     +   case SpvOpBitFieldSExtract:
>     +   case SpvOpBitFieldUExtract: {
>     +      bool swap;
>     +      unsigned src_bit_size = glsl_get_bit_size(vtn_src[0]->type);
>     +      unsigned dst_bit_size = glsl_get_bit_size(type);
>     +      nir_op op = vtn_nir_alu_op_for_spirv_opcode(b, opcode, &swap,
>     +                                                  src_bit_size,
>     dst_bit_size);
>     +      if (src[1]->bit_size != 32) {
>     +         /* Convert the Shift operand to 32 bits, which is the
>     bitsize
>     +          * supported by the NIR instruction. See discussion here:
>     +          *
>     +          *
>     https://lists.freedesktop.org/archives/mesa-dev/2018-April/193026.html
>     <https://lists.freedesktop.org/archives/mesa-dev/2018-April/193026.html>
>     +          */
>     +         src[1] = nir_build_alu(&b->nb, nir_op_u2u32, src[1],
>     NULL, NULL, NULL);
>     +      }
>     +      if (src[2]->bit_size != 32) {
>     +         /* Convert the Shift operand to 32 bits, which is the
>     bitsize
>     +          * supported by the NIR instruction. See discussion here:
>     +          *
>     +          *
>     https://lists.freedesktop.org/archives/mesa-dev/2018-April/193026.html
>     <https://lists.freedesktop.org/archives/mesa-dev/2018-April/193026.html>
>     +          */
>     +         src[2] = nir_build_alu(&b->nb, nir_op_u2u32, src[2],
>     NULL, NULL, NULL);
>     +      }
>     +      val->ssa->def = nir_build_alu(&b->nb, op, src[0], src[1],
>     src[2], src[3]);
>     +      break;
>
>
> I'm wondering if we don't want to just do something such as
>
> for (unsigned i = 0; i < nir_op_infos[op].num_inputs; i++) {
>    src_bit_size =
> nir_alu_type_get_get_type_size(nir_op_infos[op].input_types[i]);
>    if (src_bit_size && src_bit_size != src[i]->bit_size) {
>       /* Comment goes here */
>       assert(src_bit_size == 32);
>       assert(op == nir_op_ushr || op == ...);
>       src[i] = nir_u2u32(&b->nb, src[i]);
>    }
> }
>
> And then we can cover all of these cases in one go.
>  

Right, but I don't want to convert all of them. The starting value for
the loop counter should be 1 for some ops and 2 for others, so we don't
convert the operand that has the value that we will operate.

I'm going to write a patch for that. Thanks for the suggestions.

Sam

>     +   }
>     +
>         case SpvOpShiftLeftLogical:
>         case SpvOpShiftRightArithmetic:
>         case SpvOpShiftRightLogical: {
>     -- 
>     2.14.1
>
>     _______________________________________________
>     mesa-dev mailing list
>     mesa-dev at lists.freedesktop.org <mailto:mesa-dev at lists.freedesktop.org>
>     https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>     <https://lists.freedesktop.org/mailman/listinfo/mesa-dev>
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180430/951878d6/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180430/951878d6/attachment-0001.sig>