[PATCH v4 0/10] MR281: vkd3d-shader/tpf: Replace sm4 register structs with vkd3d-shader register structs, part 1.

newer
[PATCH 0/2] MR3431: wined3d: Fix...

Francisco Casas (＠fcasas)

27 Jul 2023 27 Jul '23

7:12 p.m.

Recap: I implemented relative addressing in !229, but I was suggested to handle register indexes by moving all the registers to the heap, which I did in my [nonconst-offsets-6](https://gitlab.winehq.org/fcasas/vkd3d/-/commits/nonconst-offsets-6) branch. A part of this implementation required !269, but I was asked to try to turn the sm4 register structs into the vkd3d-shader register structs instead, to get closer to a common low level IR. This is the first part of that transformation. The whole thing is in my [use_vkd3d_reg](https://gitlab.winehq.org/fcasas/vkd3d/-/commits/use_vkd3d_reg) branch. ~~This is built on top of !225 (the first two patches), so it may be good to get that upstream first.~~ --- This patch series aims to do the following replacements: ``` struct sm4_register -> struct vkd3d_shader_register struct sm4_dst_register -> struct vkd3d_shader_dst_param struct sm4_src_register -> struct vkd3d_shader_src_param ``` to get us closer to a common level IR and simplify the implementation of relative-addressing. To achieve this, the fields in the sm4 register structs are replaced with fields in the vkd3d-shader structs or removed altogether, one at the time. As can be seen when looking at the whole branch, it is possible to do this transformation without having to add additional fields to `struct vkd3d_shader_register`, by restricting each register type to a single `enum vkd3d_sm4_dimension` (and its src registers to a single `enum vkd3d_sm4_swizzle_type`) by default. The only exception we need so far is for sampler registers: They always have dimension NONE, except when used as arguments of gather instructions, in which case they have dimension VEC4 and SCALAR swizzle. This, and similar exceptions we may find in the future, can be handled using the opcode_info, as in 7/8 [81e17506](https://gitlab.winehq.org/fcasas/vkd3d/-/commit/81e17506ba2cb1fbf4d021159ab1...). -- v4: vkd3d-shader/tpf: Get rid of sm4_register.dim. vkd3d-shader/tpf: Use 's' in src_info to expect sampler register with vec4 dimension. vkd3d-shader/tpf: Separate dst register write function. vkd3d-shader/tpf: Separate src register write function. vkd3d-shader/tpf: Make register_type_table an array of structs with lookup tables. vkd3d-shader/tpf: Pass ctx to sm4_encode_register(). https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/281

Show replies by date

Conor McCarthy

27 Jul 27 Jul

7:13 p.m.

On Thu Jul 27 19:12:45 2023 +0000, Francisco Casas wrote:

...

changed this line in [version 4 of the diff](/wine/vkd3d/-/merge_requests/281/diffs?diff_id=59763&start_sha=7d2ec1025572004a085eb820999ebcc56e8f93b5#b9a696b7e2f161c9052ff1159d7d8cb2907cdeae_579_578) Done!

-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/281#note_40559

Francisco Casas (＠fcasas)

7:14 p.m.

On Thu Jul 27 19:14:05 2023 +0000, Giovanni Mascellani wrote:

...

I think we usually split the "pass `ctx` to this function" part in a dedicated patch. It feels much cleaner to me. Done!

-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/281#note_40560

Henri Verbeet (＠hverbeet)

9:37 p.m.

...

...
So it actually doesn't depend on those patches at all. :) This series might have some minor conflicts with that series, but we should simply deal with those once they actually happen; it's entirely possible for this series to go in first.

Yes. Although, I am not sure if you mean that I should drop the first patch, that just renames VKD3D_SM4_SWIZZLE_MASK4 to VKD3D_SM4_SWIZZLE_NONE. Note that this renaming is only a subset of the first patch by Conor. I think that the new name is more appropriate.

I'm not quite as convinced it's an improvement, but the broader point is that it's an unrelated change, and should go in its own MR. It happens to touch some lines that the rest of this series also touches, but that can't be the only reason to include something in a series.

...

```diff + if (ctx->profile->major_version >= 4) + { + if (!(ctx->lookup = hlsl_new_sm4_lookup_tables())) + { + vkd3d_free((void *)ctx->source_files[0]); + vkd3d_free(ctx->source_files); + return false; + } + } ```

I don't hate it, but I think we can do better. In particular, I don't think we need these lookup tables before calling hlsl_sm4_write(), and that means we can just call hlsl_new_sm4_lookup_tables() there, and keep all the lookup table bits local to tpf.c. I could imagine introducing a structure like this: ```c struct tpf_writer { struct vkd3d_bytecode_buffer buffer; struct vkd3d_sm4_lookup_tables lookup; struct hlsl_ctx *ctx; }; ``` and then passing that to e.g. write_sm4_shdr(). To put this in the broader picture, we'd ultimately like the HLSL compiler to produce a vkd3d_shader_instruction_array structure, and the TPF writer to consume that structure. Ideally the functions writing TPF then wouldn't have direct knowledge about HLSL IR, and wouldn't get passed a hlsl_ctx structure. It'll probably be a while before we get there, but I think we should at least try to avoid exposing TPF details outside tpf.c. Similarly, we probably want to avoid using hlsl_ctx in TPF writer functions that currently don't need it.

...

```diff @@ -587,6 +595,8 @@ struct vkd3d_shader_sm4_parser struct sm4_index_range_array output_index_ranges; struct sm4_index_range_array patch_constant_index_ranges;

+ struct vkd3d_sm4_lookup_tables *lookup; + struct vkd3d_shader_parser p; };

```

I don't think "lookup" needs to be a pointer here, right?

...

```diff +static const struct vkd3d_sm4_register_type_info register_type_table[] = +{ + {VKD3D_SM4_RT_TEMP, VKD3DSPR_TEMP}, + {VKD3D_SM4_RT_INPUT, VKD3DSPR_INPUT}, ... ```

Now that we're no longer accessing register_type_table[] directly, we can make it local to hlsl_new_sm4_lookup_tables().

...

```diff +static const struct vkd3d_sm4_register_type_info *get_info_from_sm4_register_type( + struct vkd3d_sm4_lookup_tables *lookup, enum vkd3d_sm4_register_type sm4_type) +{ + assert(sm4_type < VKD3D_SM4_REGISTER_TYPE_COUNT); + return lookup->rt_info_from_sm4[sm4_type]; +} ... @@ -1656,15 +1703,15 @@ static bool shader_sm4_read_param(struct vkd3d_shader_sm4_parser *priv, const ui token = *(*ptr)++;

register_type = (token & VKD3D_SM4_REGISTER_TYPE_MASK) >> VKD3D_SM4_REGISTER_TYPE_SHIFT; - if (register_type >= ARRAY_SIZE(register_type_table) - || register_type_table[register_type] == VKD3DSPR_INVALID) + register_type_info = get_info_from_sm4_register_type(priv->lookup, register_type); + if (!register_type_info) { FIXME("Unhandled register type %#x.\n", register_type); param->type = VKD3DSPR_TEMP; } ```

You can't do that. "register_type"/"sm4_type" is read from the bytecode, which is untrusted input. The existing code is perhaps not great in this regard either, but replacing the existing check with an assert makes it worse. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/281#note_40577

Francisco Casas (＠fcasas)

10:11 p.m.

...

I'm not quite as convinced it's an improvement, but the broader point is that it's an unrelated change, and should go in its own MR. It happens to touch some lines that the rest of this series also touches, but that can't be the only reason to include something in a series.

Fair point. I will drop the patch then.

...

I don't hate it, but I think we can do better. In particular, I don't think we need these lookup tables before calling hlsl_sm4_write(), and that means we can just call hlsl_new_sm4_lookup_tables() there, and keep all the lookup table bits local to tpf.c.

I thought on putting the lookup tables in `struct tpf_writer` but then I thought that it may be more futureproof to pass the ctx as argument to all the functions than the writer.

...

To put this in the broader picture, we'd ultimately like the HLSL compiler to produce a vkd3d_shader_instruction_array structure, and the TPF writer to consume that structure. Ideally the functions writing TPF then wouldn't have direct knowledge about HLSL IR, and wouldn't get passed a hlsl_ctx structure. It'll probably be a while before we get there, but I think we should at least try to avoid exposing TPF details outside tpf.c. Similarly, we probably want to avoid using hlsl_ctx in TPF writer functions that currently don't need it.

I see. Thanks, I appreciate broader pictures! I will modify the series to do that then.

...

I don't think "lookup" needs to be a pointer here, right?

That was out of necessity, because `struct vkd3d_sm4_lookup_tables` used structs only defined in tpf.c, so it had to be an incomplete type, so I had to use a pointer. I guess it will no longer be necessary (together with allocation the lookup tables in the heap) if we put in in the ~~parser~~ writer.

...

Now that we're no longer accessing register_type_table\[\] directly, we can make it local to hlsl_new_sm4_lookup_tables().

Okay.

...

You can't do that. "register_type"/"sm4_type" is read from the bytecode, which is untrusted input. The existing code is perhaps not great in this regard either, but replacing the existing check with an assert makes it worse.

Okay, I suspected such a thing after sending the series. I will make the lookups to return NULL if they are outside the table. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/281#note_40583

Henri Verbeet (＠hverbeet)

10:27 p.m.

...

...
I don't think "lookup" needs to be a pointer here, right?

That was out of necessity, because `struct vkd3d_sm4_lookup_tables` used structs only defined in tpf.c, so it had to be an incomplete type, so I had to use a pointer.

For the one in struct hlsl_ctx, sure. The comment was about the one in struct vkd3d_shader_sm4_parser though, which doesn't have that restriction; in fact, the vkd3d_sm4_lookup_tables structure is defined right above it. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/281#note_40585

880

Age (days ago)

880

Last active (days ago)

List overview

Download

15 comments

4 participants

participants (4)

Conor McCarthy
Francisco Casas
Francisco Casas (＠fcasas)
Henri Verbeet (＠hverbeet)

[PATCH v4 0/10] MR281: vkd3d-shader/tpf: Replace sm4 register structs with vkd3d-shader register structs, part 1.

tags

participants (4)