vkd3d-shader/tpf: Replace sm4 register structs with vkd3d-shader register structs, part 1.
Recap:
I implemented relative addressing in !229 (closed), but I was suggested to handle register indexes by moving all the registers to the heap, which I did in my nonconst-offsets-6 branch. A part of this implementation required !269 (closed), 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_4 branch.
This is built on top of !225 (merged) (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.
Merge request reports
Activity
requested review from @fcasas
mentioned in merge request !269 (closed)
- Resolved by Giovanni Mascellani
- Resolved by Giovanni Mascellani
- Resolved by Giovanni Mascellani
- Resolved by Giovanni Mascellani
- Resolved by Giovanni Mascellani
- Resolved by Giovanni Mascellani
- Resolved by Giovanni Mascellani
- Resolved by Giovanni Mascellani
Overall this looks fine to me, with a few minor improvement points.
The
's'
trick doesn't really thrill me, but right now I can't think of anything better and I prefer to see this moving forward rather than bikeshedding that bit (which can always be improved later, if a better solution arise at some point).+static const struct vkd3d_sm4_register_type_info *get_register_type_info( + enum vkd3d_sm4_register_type type) +{ + unsigned int i; + + for (i = 0; i < ARRAY_SIZE(register_type_table); ++i) + { + if (type == register_type_table[i].type) return ®ister_type_table[i]; + } + + return NULL; +} + +static const struct vkd3d_sm4_register_type_info *get_register_type_info_from_handler( + enum vkd3d_shader_register_type handler) +{ + unsigned int i; + + for (i = 0; i < ARRAY_SIZE(register_type_table); ++i) + { + if (handler == register_type_table[i].handler_type) return ®ister_type_table[i]; + } + + return NULL; +}
We care about parser speed at least somewhat, and looping over the table like that is probably going to make it worse. (On that note, get_opcode_info() is one of the current offenders, and we should probably just index opcode_table[] by the opcode id.) We probably care slightly less about HLSL compilation time, but I'd probably feel better about simply having separate tables for vkd3d->sm4 and sm4->vkd3d.
There may be different ways to get there, of course. The most straightforward approach would be to simply put two separate tables in tpf.c, and I think that would be fine. We could also generate those lookup tables from a single table like you have here though, either at runtime (for example, the first time get_register_type_info()/get_register_type_info_from_handler() is called) or at compile time.
I don't love the names "type" and "handler_type"; I'd probably prefer some variant of "tpf_type"/"vkd3d_type", "sm4_type"/"ir_type", or something along those lines.
The
's'
trick doesn't really thrill me, but right now I can't think of anything better and I prefer to see this moving forward rather than bikeshedding that bit (which can always be improved later, if a better solution arise at some point).Likewise; I don't think it's great, but I can live with it.
Does this series strictly depend on the first two patches by Conor?
I see, I modified get_register_type_info() and get_register_type_info_from_handler() to have static lookup arrays, initialized on the first call. If you think this solutions is fine, I can send a similar solution for get_opcode_info() as another MR.
Also, I renamed the functions to get_info_from_sm4_register_type() and get_info_from_vkd3d_register_type().
I also renamed the vkd3d_sm4_register_type_info fields as sm4_type and vkd3d_type.
Does this series strictly depend on the first two patches by Conor?
I see now that it is not as dependent as !269 (closed) was. I removed the first two patches and just replaced it with a patch that renames
VKD3D_SM4_SWIZZLE_NONE
toVKD3D_SM4_SWIZZLE_MASK4
.I see, I modified get_register_type_info() and get_register_type_info_from_handler() to have static lookup arrays, initialized on the first call. If you think this solutions is fine, I can send a similar solution for get_opcode_info() as another MR.
Unfortunately that's slightly wrong: without appropriate synchronization two different threads might try to do that initialization step concurrently of step on each other. I believe it's very improbable that anything bad happens, honestly, but it's the kind of thing I'd try to avoid. C doesn't offer, AFAICT, a fully satisfying way to do what you want to do in a DRY and self-contained way (which is part of why I didn't raise this point in my review). The ways forward I can see are these:
- Do the same thing, but use proper locking and/or atomic operations to ensure concurrent threads do the right thing. It can be tricky and it forces us to deal with threading API in a library that probably shouldn't. I'd avoid that.
- Do the same thing, but store the lookup tables in the parser context instead of in a static variable. There's a bit of CPU cycle wasting, but it's a small thing, and concurrent threads work on independent tables and do not step on each other.
- Don't do anything fancy, just write both lookup tables in the code. It's not DRY, so in principle I hate that, but given that the two tables are still rather small and C really tries to get in your way here, it still feels one of the best alternatives.
- Turn to the dark side and embrace the C way: the preprocessor! Create a file full of
X(VKD3D_SM4_RT_TEMP, VKD3DSPR_TEMP)
lines and include each time you need it definingX()
appropriately each time. Once you're done with that you start doing things like#define E } else {
and you're halfway submitting vkd3d-shader to the IOCCC. It's a great career path, though, I think you can get consultancy work from Satan himself.
Notice that with the third and fourth alternatives you can also write a
switch
statement instead of a lookup table, in theory giving your compiler even more optimization opportunities. Not sure the compiler is going to make good use of them, though.I'd go with the second or third alternative.
I see your point. AFAIK hlsl_sm4_write() always gets called by a single thread, so I guess that the problem may happen when calling the disassembler. I have to understand that code path better.
Anyways, notice that the initialization part of the functions just read from
register_type_table
, which is const, and write values tolookup
without reading it, and the values are the same regardless of which thread is writing them. So, even if many threads were to pass the!computed_lookup
check at the same time, they wouldn't be stepping on each other.I dare to say that this is one of the weird cases where parallelization doesn't require synchronization structs. Now, the question is if it is possible that the compiler does some optimization that breaks this rather special situation.
I will explore the second and third alternatives though.
I see your point. AFAIK hlsl_sm4_write() always gets called by a single thread
Why? If the client calls
vkd3d_shader_compile()
from two different threads at the same time it can happen that the twohlsl_sm4_write()
calls execute at the same time, can't it?I agree that it's probable that in practice nothing bad happens, at least on the architectures we care about, because you're just going to do aligned pointer accesses. But technically concurrent access to the same memory location is UB in C (if at least one of the accesses is for writing), so I think we'd better stay away from that. Not sure if others have different opinions on the matter.
added 8 commits
- 40557741 - vkd3d-shader/tpf: Rename VKD3D_SM4_SWIZZLE_NONE as VKD3D_SM4_SWIZZLE_MASK4.
- 71caa370 - vkd3d-shader/tpf: Use enum vkd3d_shader_register_type in sm4_register.type.
- 9efbfc18 - vkd3d-shader/tpf: Use struct vkd3d_shader_register_index in sm4_register.idx[].
- 16f99773 - vkd3d-shader/tpf: Make register_type_table an array of structs.
- 64be142f - vkd3d-shader/tpf: Separate src register write function.
- 0bc5806b - vkd3d-shader/tpf: Separate dst register write function.
- 237870cb - vkd3d-shader/tpf: Use 's' in src_info to expect sampler register with vec4 dimension.
- 1831decf - vkd3d-shader/tpf: Get rid of sm4_register.dim.
Toggle commit listDoes this series strictly depend on the first two patches by Conor?
I see now that it is not as dependent as !269 (closed) was. I removed the first two patches and just replaced it with a patch that renames
VKD3D_SM4_SWIZZLE_NONE
toVKD3D_SM4_SWIZZLE_MASK4
.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.
bool hlsl_sm4_register_from_semantic(struct hlsl_ctx *ctx, const struct hlsl_semantic *semantic, - bool output, unsigned int *type, enum vkd3d_sm4_swizzle_type *swizzle_type, bool *has_idx) + bool output, enum vkd3d_shader_register_type *type, enum vkd3d_sm4_swizzle_type *swizzle_type, bool *has_idx)
You should update the prototype in hlsl.h as well.
@@ -2555,7 +2555,8 @@ bool hlsl_sm4_register_from_semantic(struct hlsl_ctx *ctx, const struct hlsl_sem && output == register_table[i].output && ctx->profile->type == register_table[i].shader_type) { - *type = register_table[i].type; + if (type) + *type = register_table[i].type; if (swizzle_type) *swizzle_type = register_table[i].swizzle_type; *has_idx = register_table[i].has_idx; @@ -2652,7 +2653,6 @@ static void write_sm4_signature(struct hlsl_ctx *ctx, struct dxbc_writer *dxbc, LIST_FOR_EACH_ENTRY(var, &ctx->extern_vars, struct hlsl_ir_var, extern_entry) { unsigned int width = (1u << var->data_type->dimx) - 1, use_mask; - enum vkd3d_sm4_register_type type; uint32_t usage_idx, reg_idx; D3D_NAME usage; bool has_idx; @@ -2666,14 +2666,13 @@ static void write_sm4_signature(struct hlsl_ctx *ctx, struct dxbc_writer *dxbc, continue; usage_idx = var->semantic.index; - if (hlsl_sm4_register_from_semantic(ctx, &var->semantic, output, &type, NULL, &has_idx)) + if (hlsl_sm4_register_from_semantic(ctx, &var->semantic, output, NULL, NULL, &has_idx)) { reg_idx = has_idx ? var->semantic.index : ~0u; } else { assert(var->regs[HLSL_REGSET_NUMERIC].allocated); - type = VKD3D_SM4_RT_INPUT; reg_idx = var->regs[HLSL_REGSET_NUMERIC].id; }
I don't dislike this, but it does seem like somewhat of a separate change.
+ VKD3D_SM4_REGISTER_TYPE_LAST = VKD3D_SM5_RT_OUTPUT_STENCIL_REF,
We'd typically add something like VKD3D_SM4_REGISTER_TYPE_COUNT. E.g., VKD3D_SHADER_TYPE_COUNT.
- Do the same thing, but use proper locking and/or atomic operations to ensure concurrent threads do the right thing. It can be tricky and it forces us to deal with threading API in a library that probably shouldn't. I'd avoid that.
- Do the same thing, but store the lookup tables in the parser context instead of in a static variable. There's a bit of CPU cycle wasting, but it's a small thing, and concurrent threads work on independent tables and do not step on each other.
- Don't do anything fancy, just write both lookup tables in the code. It's not DRY, so in principle I hate that, but given that the two tables are still rather small and C really tries to get in your way here, it still feels one of the best alternatives.
- Turn to the dark side and embrace the C way: the preprocessor! Create a file full of
X(VKD3D_SM4_RT_TEMP, VKD3DSPR_TEMP)
lines and include each time you need it definingX()
appropriately each time. Once you're done with that you start doing things like#define E } else {
and you're halfway submitting vkd3d-shader to the IOCCC. It's a great career path, though, I think you can get consultancy work from Satan himself.
Notice that with the third and fourth alternatives you can also write a
switch
statement instead of a lookup table, in theory giving your compiler even more optimization opportunities. Not sure the compiler is going to make good use of them, though.I'd go with the second or third alternative.
Yeah. I'd go with the second option.
It's probably not that hard to make the first option work with either something like "__attribute__((constructor))" or something like pthread_once(), although there may be a few pitfalls. I doubt it's worth the effort at this point though.
+ VKD3D_SM4_DIMENSION_INVALID = ~0u,
Do we really need this? We use it in register_type_table[] for VKD3DSPR_IMMCONST and VKD3DSPR_IMMCONST64, but then overwrite it for VKD3DSPR_IMMCONST. I'm not sure it adds that much over using e.g. VKD3D_SM4_DIMENSION_NONE or VKD3D_SM4_DIMENSION_VEC4 for these.
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.
You should update the prototype in hlsl.h as well.
Done.
I don't dislike this, but it does seem like somewhat of a separate change.
I split it as a separate patch.
We'd typically add something like VKD3D_SM4_REGISTER_TYPE_COUNT. E.g., VKD3D_SHADER_TYPE_COUNT.
I changed it, seems better.
Yeah. I'd go with the second option.
I implemented the second option. It requires the lookup tables to be in both struct hlsl_ctx and struct vkd3d_shader_sm4_parser, and passing the context or the lookup tables in a lot of functions in tpf.c, which may make option 3 more preferable. A good thing though is that these changes would allow to use the same strategy to speed up get_opcode_info().
Do we really need this? We use it in register_type_table[] for VKD3DSPR_IMMCONST and VKD3DSPR_IMMCONST64, but then overwrite it for VKD3DSPR_IMMCONST. I'm not sure it adds that much over using e.g. VKD3D_SM4_DIMENSION_NONE or VKD3D_SM4_DIMENSION_VEC4 for these.
Good point. VKD3D_SM4_DIMENSION_INVALID was to remind that the dimension depends on the circumstances and should be overwritten by them. I changed it to VKD3D_SM4_DIMENSION_NONE, but I wrote it in the table as a mere 0, as a reminder of that.
VKD3D_SM4_DIMENSION_INVALID was also meant to trigger an assertion fail in case we were emitting a VKD3DSPR_IMMCONST64 because we didn't have code were we were explicitly setting the dimension for this register type. After some tests, I see that VKD3DSPR_IMMCONST64 always uses VKD3D_SM4_DIMENSION_VEC4, even for scalar doubles, so I just put that in the table.
added 8 commits
- 342c7f9e - vkd3d-shader/tpf: Use enum vkd3d_shader_register_type in sm4_register.type.
- 8f210508 - vkd3d-shader/tpf: Allow passing NULL register type on hlsl_sm4_register_from_semantic().
- e25166b1 - vkd3d-shader/tpf: Use struct vkd3d_shader_register_index in sm4_register.idx[].
- 3c24a513 - vkd3d-shader/tpf: Make register_type_table an array of structs.
- c5bde1b0 - vkd3d-shader/tpf: Separate src register write function.
- 9dd894b6 - vkd3d-shader/tpf: Separate dst register write function.
- 351c4418 - vkd3d-shader/tpf: Use 's' in src_info to expect sampler register with vec4 dimension.
- 7d2ec102 - vkd3d-shader/tpf: Get rid of sm4_register.dim.
Toggle commit list- Resolved by Giovanni Mascellani
- Resolved by Giovanni Mascellani
added 6 commits
- 46370507 - vkd3d-shader/tpf: Pass ctx to sm4_encode_register().
- 6e454b07 - vkd3d-shader/tpf: Make register_type_table an array of structs with lookup tables.
- a4235986 - vkd3d-shader/tpf: Separate src register write function.
- 31786dde - vkd3d-shader/tpf: Separate dst register write function.
- 9d19952c - vkd3d-shader/tpf: Use 's' in src_info to expect sampler register with vec4 dimension.
- 281373b1 - vkd3d-shader/tpf: Get rid of sm4_register.dim.
Toggle commit listSo 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.
+ 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:
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.
@@ -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?
+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().
+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.
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
parserwriter.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.
Edited by Francisco CasasI 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.Sorry, meant, I thought on putting the lookup tables in
struct dxbc_writer
here. Well, it doesn't matter now. I created the newstruct tpf_writer
.
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.
added 9 commits
- fe6c3c59 - vkd3d-shader/tpf: Use enum vkd3d_shader_register_type in sm4_register.type.
- 70b682cf - vkd3d-shader/tpf: Allow passing NULL register type on hlsl_sm4_register_from_semantic().
- 5d448fc4 - vkd3d-shader/tpf: Use struct vkd3d_shader_register_index in sm4_register.idx[].
- 8e68aad8 - vkd3d-shader/tpf: Introduce struct tpf_writer to group sm4 write arguments.
- 126f01a3 - vkd3d-shader/tpf: Make register_type_table an array of structs with lookup tables.
- b01283d0 - vkd3d-shader/tpf: Separate src register write function.
- c0de621f - vkd3d-shader/tpf: Separate dst register write function.
- 88a449cc - vkd3d-shader/tpf: Use 's' in src_info to expect sampler register with vec4 dimension.
- 162ab1f0 - vkd3d-shader/tpf: Get rid of sm4_register.dim.
Toggle commit listremoved review request for @fcasas
This essentially works for me, but note that it doesn't currently apply cleanly. I do have some comments, although they're fairly minor in the scheme of things:
From 8e68aad88d82ca9d61d6ef8525f6604c885bf7f9 Mon Sep 17 00:00:00 2001 From: Francisco Casas <fcasas@codeweavers.com> Date: Wed, 19 Jul 2023 12:19:02 -0400 Subject: [PATCH 4/9] vkd3d-shader/tpf: Introduce struct tpf_writer to group sm4 write arguments. We will add register information lookup tables on this struct later. They will be used by sm4_encode_register(), and thus, they will be required by write_sm4_instruction(). --- libs/vkd3d-shader/tpf.c | 459 ++++++++++++++++++++-------------------- 1 file changed, 228 insertions(+), 231 deletions(-)
I generally prefer doing changes like these function by function; start in write_sm4_shdr(), and then push down from there. That would also make potential conflicts easier to handle/avoid. I think this change is straightforward enough that I'm not going to require splitting the commit, but please take it into future consideration.
+ tpf.ctx = ctx; + tpf.buffer = &buffer;
That works, but it might be nice to introduce a tpf_writer_init() function as well, particularly once the lookup tables are introduced.
From 126f01a3b9e622d25aad053d51bd999f1b6e60dd Mon Sep 17 00:00:00 2001 From: Francisco Casas <fcasas@codeweavers.com> Date: Wed, 19 Jul 2023 12:19:02 -0400 Subject: [PATCH 5/9] vkd3d-shader/tpf: Make register_type_table an array of structs with lookup tables. --- libs/vkd3d-shader/tpf.c | 170 +++++++++++++++-------- libs/vkd3d-shader/vkd3d_shader_private.h | 2 + 2 files changed, 111 insertions(+), 61 deletions(-)
Somewhat like patch 4/9, this could probably be split in sm4_encode_register() and shader_sm4_read_param() parts.
+ for (i = 0; i < ARRAY_SIZE(register_type_table); ++i) + { + t = register_type_table[i].sm4_type; + lookup->register_type_info_from_sm4[t] = ®ister_type_table[i]; + t = register_type_table[i].vkd3d_type; + lookup->register_type_info_from_vkd3d[t] = ®ister_type_table[i]; + }
This works too, but perhaps something like the following is nicer?
const struct vkd3d_sm4_register_type_info *info; ... for (i = 0; i < ARRAY_SIZE(register_type_table); ++i) { info = ®ister_type_table[i]; lookup->register_type_info_from_sm4[info->sm4_type] = info; lookup->register_type_info_from_vkd3d[info->vkd3d_type] = info; }
This essentially works for me, but note that it doesn't currently apply cleanly.
Yep, I have been told to leave the rebases for after the MR is agreed upon.
I generally prefer doing changes like these function by function; start in write_sm4_shdr(), and then push down from there. That would also make potential conflicts easier to handle/avoid. I think this change is straightforward enough that I'm not going to require splitting the commit, but please take it into future consideration.
Okay, it didn't occur to me doing this from the top-down.
That works, but it might be nice to introduce a tpf_writer_init() function as well, particularly once the lookup tables are introduced.
Done.
This works too, but perhaps something like the following is nicer?
const struct vkd3d_sm4_register_type_info *info; ... for (i = 0; i < ARRAY_SIZE(register_type_table); ++i) { info = ®ister_type_table[i]; lookup->register_type_info_from_sm4[info->sm4_type] = info; lookup->register_type_info_from_vkd3d[info->vkd3d_type] = info; }
Done.
added 6 commits
- 5e48e60c - vkd3d-shader/tpf: Introduce struct tpf_writer to group sm4 write arguments.
- a3f48c67 - vkd3d-shader/tpf: Make register_type_table an array of structs with lookup tables.
- b1f423cf - vkd3d-shader/tpf: Separate src register write function.
- 5a503c1a - vkd3d-shader/tpf: Separate dst register write function.
- 87f16436 - vkd3d-shader/tpf: Use 's' in src_info to expect sampler register with vec4 dimension.
- 34c6471a - vkd3d-shader/tpf: Get rid of sm4_register.dim.
Toggle commit listadded 60 commits
-
34c6471a...b4bb3931 - 51 commits from branch
wine:master
- 616f8af3 - vkd3d-shader/tpf: Use enum vkd3d_shader_register_type in sm4_register.type.
- 8f5348d4 - vkd3d-shader/tpf: Allow passing NULL register type on hlsl_sm4_register_from_semantic().
- 35b78535 - vkd3d-shader/tpf: Use struct vkd3d_shader_register_index in sm4_register.idx[].
- e7033422 - vkd3d-shader/tpf: Introduce struct tpf_writer to group sm4 write arguments.
- d23bb727 - vkd3d-shader/tpf: Make register_type_table an array of structs with lookup tables.
- d6382962 - vkd3d-shader/tpf: Separate src register write function.
- 36f2486e - vkd3d-shader/tpf: Separate dst register write function.
- 4a581369 - vkd3d-shader/tpf: Use 's' in src_info to expect sampler register with vec4 dimension.
- 3e36cb97 - vkd3d-shader/tpf: Get rid of sm4_register.dim.
Toggle commit list-
34c6471a...b4bb3931 - 51 commits from branch
Most of this looks fine to me, but I have my doubts about 9/9. What the old code calls "immconst_type" is the dimension; I don't think that it's meaningfully different, and I think that we should quite possibly be making vsir look more like the hlsl_sm4 layer in this respect—that is, rename "immconst_type" to "dimension" and parse and specify it everywhere.
If we do that, that takes away the impetus for putting it in a table.
If we don't, we should probably get the table entries right from the beginning. Most of those entries are marked VEC4, which at least seems surprising. Also, marking IMMCONST as 0 isn't great, because 0 is NONE.
Leaving that last patch it off the series for now may be prudent.
Most of this looks fine to me, but I have my doubts about 9/9. What the old code calls "immconst_type" is the dimension; I don't think that it's meaningfully different, and I think that we should quite possibly be making vsir look more like the hlsl_sm4 layer in this respect—that is, rename "immconst_type" to "dimension" and parse and specify it everywhere.
Yeah. The reason it ended up being like that is that most registers have an implicit dimension; vec4 for most, but scalar for the ones from shader_sm4_is_scalar_register(). (And looking more closely at 9/9, I see some inconsistencies with shader_sm4_is_scalar_register()...) Immediate constants were the exception for not having such an implied dimension.
Leaving that last patch it off the series for now may be prudent.
I wasn't too worried about 9/9, but you're probably right that it's not the direction we want to go in. More generally it's of course also true that it tends to be faster to get two small series in than it is to get one large one in...
Most of this looks fine to me, but I have my doubts about 9/9. What the old code calls "immconst_type" is the dimension; I don't think that it's meaningfully different, and I think that we should quite possibly be making vsir look more like the hlsl_sm4 layer in this respect—that is, rename "immconst_type" to "dimension" and parse and specify it everywhere.
If we do that, that takes away the impetus for putting it in a table.
I see. If we replace
vkd3d_shader_register.immconst_type
withvkd3d_shader_register.dimension
and always define it, I think we could drop 8/9 too then.If we don't, we should probably get the table entries right from the beginning. Most of those entries are marked VEC4, which at least seems surprising.
Well, I made the table so that we replicate the current behavior, which is indeed writing VEC4 often.
Also, marking IMMCONST as 0 isn't great, because 0 is NONE.
Yep, I added a
VKD3D_SM4_DIMENSION_INVALID
in previous versions, but I removed it, I used 0 as a reminder that this was the exception to be handled according to the circumstances.