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

[Zebediah Figura]

On 11/29/21 10:35, Francisco Casas wrote:
> 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.
> 

I wouldn't say that, in fact we often err on the side of premature 
generalization :-)

But at the same time we try to avoid dead code, and I would agree that 
things like encode_int() aren't really helping. I think it can be an 
implicit assertion that we're passing valid values.