[PATCH vkd3d 2/3] vkd3d-shader/hlsl: Take writemask into account when emitting a SM4 constant.

[Zebediah Figura (she/her)]

On 11/18/21 11:14, Matteo Bruni wrote:
> On Thu, Nov 18, 2021 at 3:44 AM Zebediah Figura (she/her)
> <zfigura at codeweavers.com> wrote:
>>
>> On 11/17/21 03:52, Matteo Bruni wrote:
>>> On Tue, Nov 16, 2021 at 10:23 AM Giovanni Mascellani
>>> <gmascellani at codeweavers.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 15/11/21 18:48, Zebediah Figura (she/her) wrote:
>>>>> This is probably quibbling, but how about this?
>>>>>
>>>>> j = 0;
>>>>> for (i = 0; i < 4; ++i)
>>>>> {
>>>>>        if (instr.dsts[0].writemask & (1u << i))
>>>>>            instr.srcs[0].reg.immconst_uint[i] = constant->value[j++].u;
>>>>> }
>>>>>
>>>>> Feels a bit simpler to me.
>>>>
>>>> I agree it's simpler, but as written it's also wrong. :-)
>>>>
>>>> The problem is that when dimx == 1, we set the register to have
>>>> dimension VKD3D_SM4_DIMENSION_SCALAR, which means that the constant must
>>>> be written in the first register, no matter the writemask.
>>>
>>> That's probably right but it raises all kinds of red flags for me. Why
>>> is dimx == 1 such a special case (aside from the SCALAR / VEC4
>>> dimension)? We're basically saying that we ignore the writemask for
>>> scalar constants. Why?
>>>
>>> The code being convoluted, or mostly duplicated, is a consequence of
>>> the above. It feels like we should fix that instead.
>>>
>>
>> dimx == 1 is a special case because it's a special case in DXBC. We're
>> not ignoring the writemask; it's just that for scalars the writemask is
>> only reflected in the destination register, whereas for vectors it's
>> reflected in the source registers as well.
>>
>> As Giovanni puts it, write_sm4_instruction() is mostly just a
>> serialization helper. hlsl_ir_node structures categorically contain
>> "unmapped" data—so the "writemask" field of hlsl_reg is a misnomer; on
>> the other hand, not all source registers are "mapped" (e.g. ld/sample
>> coords) so we can't categorically do mapping in write_sm4_instruction().
>> Hence we do that translation in the individual callers, like this one.
>>
>> We should be doing a similar thing for normal arithmetic register
>> swizzles, but we currently aren't.
> 
> What I'm getting at is this kind of half one thing, half the opposite
> is bad. I think we should move towards eventually making struct
> sm4_instruction a proper IR and every time there is a chance of making
> progress in that direction we should take it.
> 

I don't think I see why this is half of anything. We have to translate 
between the hlsl_ir representation of swizzles and the DXBC 
representation of swizzles somewhere, why not here?

I'm not sure if we'd want the SM4 IR to use mapped swizzles or not, but 
either way, I don't see how this commit goes in the wrong direction.