[PATCH vkd3d v3 01/12] vkd3d-shader/hlsl: Add support for sm4 instruction modifiers.

[Francisco Casas]

December 17, 2021 6:21 PM, "Zebediah Figura (she/her)" <zfigura at codeweavers.com> wrote:

> On 12/17/21 13:12, Francisco Casas wrote:
> 
>> Signed-off-by: Francisco Casas <fcasas at codeweavers.com>
>> ---
>> libs/vkd3d-shader/hlsl_sm4.c | 54 ++++++++++++++++++++++++++++++++++++
>> 1 file changed, 54 insertions(+)
>> diff --git a/libs/vkd3d-shader/hlsl_sm4.c b/libs/vkd3d-shader/hlsl_sm4.c
>> index 2458018f..07bbd376 100644
>> --- a/libs/vkd3d-shader/hlsl_sm4.c
>> +++ b/libs/vkd3d-shader/hlsl_sm4.c
>> @@ -727,6 +727,46 @@ static enum vkd3d_sm4_resource_type sm4_resource_dimension(const struct
>> hlsl_typ
>> }
>> }
>>> +struct sm4_instruction_modifier
>> +{
>> + enum vkd3d_sm4_instruction_modifier type;
>> +
>> + union
>> + {
>> + struct
>> + {
>> + int u,v,w;
>> + } aoffimmi;
>> + };
>> +};
>> +
>> +static uint32_t sm4_encode_instruction_modifier(struct sm4_instruction_modifier imod)
> 
> This should be a const pointer instead, I think.

Okay, 
Is there any reason besides avoiding copying in the pass-by-value?
Or is it because it looks more consistent with the rest of the code?


>> +{
>> + uint32_t word = 0;
>> +
>> + word |= VKD3D_SM4_MODIFIER_MASK & imod.type;
>> +
>> + switch (imod.type)
>> + {
>> + case VKD3D_SM4_MODIFIER_AOFFIMMI:
>> + assert(-8 <= imod.aoffimmi.u && imod.aoffimmi.u <= 7);
>> + assert(-8 <= imod.aoffimmi.v && imod.aoffimmi.v <= 7);
>> + assert(-8 <= imod.aoffimmi.w && imod.aoffimmi.w <= 7);
>> + word |= ((uint32_t)imod.aoffimmi.u & 0xf) << VKD3D_SM4_AOFFIMMI_U_SHIFT;
>> + word |= ((uint32_t)imod.aoffimmi.v & 0xf) << VKD3D_SM4_AOFFIMMI_V_SHIFT;
>> + word |= ((uint32_t)imod.aoffimmi.w & 0xf) << VKD3D_SM4_AOFFIMMI_W_SHIFT;
>> + break;
>> +
>> + default:
>> + FIXME("Unhandled instruction modifier %#x.\n", imod.type);
>> + return 0;
>> + break;
>> + }
>> +
>> + return word;
>> +}
>> +
>> +
>> struct sm4_register
>> {
>> enum vkd3d_sm4_register_type type;
>> @@ -741,6 +781,9 @@ struct sm4_instruction
>> {
>> enum vkd3d_sm4_opcode opcode;
>>> + struct sm4_instruction_modifier modifiers[1];
>> + unsigned int modifier_count;
>> +
>> struct
>> {
>> struct sm4_register reg;
>> @@ -913,6 +956,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)
>> @@ -920,8 +964,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);
>> +
> 
> Ech, this is hard to read, how about a simpler:
> 
> if (instr->modifier_count > 0)
> token |= VKD3D_SM4_INSTRUCTION_MODIFIER;
> 
> Same thing in the loop below.
> 
>> 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);
>> + }
>> +
>> for (i = 0; i < instr->dst_count; ++i)
>> {
>> token = sm4_encode_register(&instr->dsts[i].reg);

Sure.