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

[Francisco Casas]

Signed-off-by: Francisco Casas <fcasas(a)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)
+{
+    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);
+
     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);
-- 
2.25.1

[Zebediah Figura (she/her)]

On 12/17/21 13:12, Francisco Casas wrote:
> Signed-off-by: Francisco Casas <fcasas(a)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.
> +{
> +    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);

[Francisco Casas]

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

> On 12/17/21 13:12, Francisco Casas wrote:
> 
>> Signed-off-by: Francisco Casas <fcasas(a)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.

[Zebediah Figura (she/her)]

On 12/20/21 06:25, Francisco Casas wrote:
> December 17, 2021 6:21 PM, "Zebediah Figura (she/her)" <zfigura(a)codeweavers.com> wrote:
> 
>> On 12/17/21 13:12, Francisco Casas wrote:
>>
>>> Signed-off-by: Francisco Casas <fcasas(a)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?

Yes, I think in general we want to avoid pass-by-value. There are some 
examples of it elsewhere in the code but they should probably be fixed.

[Matteo Bruni]

On Fri, Dec 17, 2021 at 10:21 PM Zebediah Figura (she/her)
<zfigura(a)codeweavers.com> wrote:
> On 12/17/21 13:12, Francisco Casas wrote:
> > Signed-off-by: Francisco Casas <fcasas(a)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.
> > +{
> > +    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;

Or even

if (instr->modifier_count)
    token |= VKD3D_SM4_INSTRUCTION_MODIFIER;

[Giovanni Mascellani]

Hi,

Il 19/01/22 13:02, Matteo Bruni ha scritto:
> > Ech, this is hard to read, how about a simpler:
> >
> > if (instr->modifier_count > 0)
> >           token |= VKD3D_SM4_INSTRUCTION_MODIFIER;
> > Or even
>
> if (instr->modifier_count)
>      token |= VKD3D_SM4_INSTRUCTION_MODIFIER;

Not a big deal, but between these two I like Zeb's version more, in that 
it makes more explicit that we're dealing with a number and not with a 
boolean condition.

Giovanni.

[Henri Verbeet]

On Wed, 19 Jan 2022 at 13:48, Giovanni Mascellani
<gmascellani(a)codeweavers.com> wrote:
> Il 19/01/22 13:02, Matteo Bruni ha scritto:
> > > Ech, this is hard to read, how about a simpler:
> > >
> > > if (instr->modifier_count > 0)
> > >           token |= VKD3D_SM4_INSTRUCTION_MODIFIER;
> > > Or even
> >
> > if (instr->modifier_count)
> >      token |= VKD3D_SM4_INSTRUCTION_MODIFIER;
>
> Not a big deal, but between these two I like Zeb's version more, in that
> it makes more explicit that we're dealing with a number and not with a
> boolean condition.

Actually, that's the reason I prefer the other variant; "Does the
instruction have modifiers?" vs "Is the number of modifiers larger
than <x>?".

[Giovanni Mascellani]

Hi,

Il 19/01/22 14:06, Henri Verbeet ha scritto:
> On Wed, 19 Jan 2022 at 13:48, Giovanni Mascellani
> <gmascellani(a)codeweavers.com> wrote:
> > Il 19/01/22 13:02, Matteo Bruni ha scritto:
> > > > Ech, this is hard to read, how about a simpler:
> > > >
> > > > if (instr->modifier_count > 0)
> > > >            token |= VKD3D_SM4_INSTRUCTION_MODIFIER;
> > > > Or even
> > >
> > > if (instr->modifier_count)
> > >       token |= VKD3D_SM4_INSTRUCTION_MODIFIER;
> >
> > Not a big deal, but between these two I like Zeb's version more, in that
> > it makes more explicit that we're dealing with a number and not with a
> > boolean condition.
>
> Actually, that's the reason I prefer the other variant; "Does the
> instruction have modifiers?" vs "Is the number of modifiers larger
> than <x>?".

Well, I'd use "!=" instead of ">".

Giovanni.