[PATCH vkd3d 1/4] vkd3d-shader/hlsl: Add support for sm4 instruction modifiers.

[Francisco Casas]

Hello,

November 29, 2021 12:25 PM, "Giovanni Mascellani" <gmascellani at codeweavers.com> wrote:

> 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.

Okay, I will remove encode_int() and change it.

> 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).

Okay, I will put an assertion then.

>> @@ -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.

It seemed enough for future uses, in the shaders compiled with the native compiler it is 
common to see instructions with 2 or 3. 
But I will leave it at 1.
 
>> +
>> 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.

Okay. 

I will steer more towards the policy of "premature generalization is the root of all evil" in future patches then.

Francisco.