On 11/18/21 11:14, Matteo Bruni wrote:
On Thu, Nov 18, 2021 at 3:44 AM Zebediah Figura (she/her) zfigura@codeweavers.com wrote:
On 11/17/21 03:52, Matteo Bruni wrote:
On Tue, Nov 16, 2021 at 10:23 AM Giovanni Mascellani gmascellani@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.