[Mesa-dev] [PATCH 1/3] spirv: fix OpBitcast when the src and dst bitsize are different
Connor Abbott
cwabbott0 at gmail.com
Sat Jul 1 02:19:29 UTC 2017
On Thu, Jun 29, 2017 at 8:00 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
> 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?
Probably not unless we need this elsewhere. I think the other places
using these instructions already know what size they want to convert
to/from, and don't need anything this complicated/generic.
>
> --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
>
>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
More information about the mesa-dev
mailing list