On 30/04/18 20:51, Jason Ekstrand wrote: > On Mon, Apr 30, 2018 at 1:46 AM, Samuel Iglesias Gonsálvez > <sigles...@igalia.com <mailto:sigles...@igalia.com>> wrote: > > On 26/04/18 18:14, Jason Ekstrand wrote: >> >> >> On Thu, Apr 26, 2018 at 2:24 AM, Samuel Iglesias Gonsálvez >> <sigles...@igalia.com <mailto:sigles...@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 >> <sigles...@igalia.com <mailto:sigles...@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 believe the sources you don't want to convert will have src_bit_size > == 0. >
Right. Just sent a new version of the patch.
Thanks,
Sam
> --Jason
>
>
> 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@lists.freedesktop.org
>> <mailto:mesa-dev@lists.freedesktop.org>
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>> <https://lists.freedesktop.org/mailman/listinfo/mesa-dev>
>>
>>
>
>
signature.asc
Description: OpenPGP digital signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
