[PATCH vkd3d 1/4] vkd3d-shader/hlsl: Add support for sm4 instruction modifiers.
Giovanni Mascellani
gmascellani at codeweavers.com
Mon Nov 29 09:25:15 CST 2021
Hi,
On 26/11/21 16:40, Francisco Casas wrote:
> +static uint32_t sm4_encode_instruction_modifier(struct sm4_instruction_modifier imod)
> +{
> + int overflow_u, overflow_v, overflow_w;
> + uint32_t word = 0;
> +
> + word |= VKD3D_SM4_MODIFIER_MASK & imod.type;
> +
> + switch (imod.type)
> + {
> + case VKD3D_SM4_MODIFIER_AOFFIMMI:
> + word |= encode_int(imod.aoffimmi.u, 4, &overflow_u) << VKD3D_SM4_AOFFIMMI_U_SHIFT;
> + word |= encode_int(imod.aoffimmi.v, 4, &overflow_v) << VKD3D_SM4_AOFFIMMI_V_SHIFT;
> + word |= encode_int(imod.aoffimmi.w, 4, &overflow_w) << VKD3D_SM4_AOFFIMMI_W_SHIFT;
To me encode_int() feels a bit overkill: it is way more general then we
need, and it's a bit complicated to check. I'd just use imod.aoffimmi.u
& 0xf.
> + if (overflow_u || overflow_v || overflow_w)
> + WARN("aoffimmi (%d,%d,%d) out of representable range -8 to 7.\n",
> + imod.aoffimmi.u, imod.aoffimmi.v, imod.aoffimmi.w);
From later patches it seems that you're already checking that offsets
have legal values before calling write_sm4_instruction (which is
sensible). Then you don't need to check the same thing here. Or, if you
want to check it again, it makes sense to make it an assertion (and, in
the same philosophy as above, you can just check that the number is
between 8 and 7, without caring about a world in which is aoffimmi is
expression with something else than 4 bits).
> @@ -741,6 +782,9 @@ struct sm4_instruction
> {
> enum vkd3d_sm4_opcode opcode;
>
> + struct sm4_instruction_modifier modifiers[4];
> + unsigned int modifier_count;
Why 4? AFAIU at least for the moment we're not using more than one.
> +
> struct
> {
> struct sm4_register reg;
> @@ -901,6 +945,7 @@ static void write_sm4_instruction(struct vkd3d_bytecode_buffer *buffer, const st
> uint32_t token = instr->opcode;
> unsigned int size = 1, i, j;
>
> + size += instr->modifier_count;
> for (i = 0; i < instr->dst_count; ++i)
> size += sm4_register_order(&instr->dsts[i].reg);
> for (i = 0; i < instr->src_count; ++i)
> @@ -908,8 +953,18 @@ static void write_sm4_instruction(struct vkd3d_bytecode_buffer *buffer, const st
> size += instr->idx_count;
>
> token |= (size << VKD3D_SM4_INSTRUCTION_LENGTH_SHIFT);
> +
> + token |= ((0 < instr->modifier_count) << 31);
> +
> put_u32(buffer, token);
>
> + for (i = 0; i < instr->modifier_count; i++)
> + {
> + token = sm4_encode_instruction_modifier(instr->modifiers[i]);
> + token |= ((i + 1 < instr->modifier_count) << 31);
> + put_u32(buffer, token);
> + }
> +
This (and the signature of sm4_encode_instruction_modifier) assume that
each instruction modifier takes exactly one dword. I don't know if this
is true, and even if it's not it makes sense to keep the assumption
while we don't have another case to handle. Just mentioning to be aware
of it.
Giovanni.
More information about the wine-devel
mailing list