Signed-off-by: Francisco Casas fcasas@codeweavers.com --- include/private/vkd3d_common.h | 24 +++++++++++++++ libs/vkd3d-shader/hlsl_sm4.c | 55 ++++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+)
diff --git a/include/private/vkd3d_common.h b/include/private/vkd3d_common.h index b7ce9ae4..2724a370 100644 --- a/include/private/vkd3d_common.h +++ b/include/private/vkd3d_common.h @@ -238,4 +238,28 @@ static inline void vkd3d_parse_version(const char *version, int *major, int *min
HRESULT hresult_from_vkd3d_result(int vkd3d_result);
+ +/* Encode a signed integer using two's complement representation in the given number of bits. + * Assumes that the machine uses two's complement too. + * *out_overflow is set to 1 in case the signed integer is out of the range of representable values. + */ +static inline uint32_t encode_int(int32_t v, uint32_t n_bits, int *out_overflow) +{ + uint32_t mask, umask; + uint32_t enc = (uint32_t) v; + + *out_overflow = 0; + + if (n_bits < 32) + { + umask = (1 << (n_bits - 1)) - 1; + *out_overflow = (v >= 0)? !!(enc & ~umask) : !!(~enc & ~umask); + + mask = (1 << n_bits) - 1; + enc = enc & mask; + } + return enc; +} + + #endif /* __VKD3D_COMMON_H */ diff --git a/libs/vkd3d-shader/hlsl_sm4.c b/libs/vkd3d-shader/hlsl_sm4.c index c0c26f80..9c500f8e 100644 --- a/libs/vkd3d-shader/hlsl_sm4.c +++ b/libs/vkd3d-shader/hlsl_sm4.c @@ -727,6 +727,47 @@ 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) +{ + 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; + 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); + 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 +782,9 @@ struct sm4_instruction { enum vkd3d_sm4_opcode opcode;
+ struct sm4_instruction_modifier modifiers[4]; + unsigned int modifier_count; + 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); + } + for (i = 0; i < instr->dst_count; ++i) { token = sm4_encode_register(&instr->dsts[i].reg);
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.
Hello,
November 29, 2021 12:25 PM, "Giovanni Mascellani" gmascellani@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.
On 11/29/21 10:35, Francisco Casas wrote:
Hello,
November 29, 2021 12:25 PM, "Giovanni Mascellani" gmascellani@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.
-
Francisco Casas -
Giovanni Mascellani -
Zebediah Figura