[PATCH 0/1] MR236: vkd3d-shader/d3dbc: Fix struct declarations in the CTBA.
This commit is part of a series of commits intended to remove all uses of hlsl_type_get_regset(). However, I think it deserves to be upstreamed sooner since it solves a rather important SM1 regression (explained below) introduced in e0031d2a1f40792ac85619a495bf5197f248b0e1 . --- In SM1 we can expect all variables to always belong to a single regset. structs in particular, should always be allocated to HLSL_REGSET_NUM, since they are only allowed if all their components are numeric. We are not covering the structs case because of the use of hlsl_type_get_regset(), which is currently not defined for structs. So the current shader ```hlsl struct { float4 a; float4 b; } apple; float4 main() : sv_target { return apple.a + apple.b; } ``` fails with ``` vkd3d/libs/vkd3d-shader/hlsl.c:224: Aborting, reached unreachable code. ``` The solution is to iterate over all regsets to find the one where the variable is allocated (if any), and ignore all others. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/236
From: Francisco Casas <fcasas(a)codeweavers.com> In SM1 we can expect all variables to always belong to a single regset. structs in particular, should always be allocated to HLSL_REGSET_NUM, since they are only allowed if all their components are numeric. We are not covering the structs case because of the use of hlsl_type_get_regset(), which is currently not defined for structs. So the current shader struct { float4 a; float4 b; } apple; float4 main() : sv_target { return apple.a + apple.b; } fails with vkd3d/libs/vkd3d-shader/hlsl.c:224: Aborting, reached unreachable code. The solution is to iterate over all regsets to find the one where the variable is allocated (if any), and ignore all others. --- libs/vkd3d-shader/d3dbc.c | 36 +++++++++++++++++++++++------------- 1 file changed, 23 insertions(+), 13 deletions(-) diff --git a/libs/vkd3d-shader/d3dbc.c b/libs/vkd3d-shader/d3dbc.c index c35f8ca0f..712613ac1 100644 --- a/libs/vkd3d-shader/d3dbc.c +++ b/libs/vkd3d-shader/d3dbc.c @@ -1281,10 +1281,13 @@ static void write_sm1_uniforms(struct hlsl_ctx *ctx, struct vkd3d_bytecode_buffe LIST_FOR_EACH_ENTRY(var, &ctx->extern_vars, struct hlsl_ir_var, extern_entry) { - enum hlsl_regset regset = hlsl_type_get_regset(var->data_type); + unsigned int r; - if (!var->semantic.name && var->regs[regset].allocated) + for (r = 0; r <= HLSL_REGSET_LAST; ++r) { + if (var->semantic.name || !var->regs[r].allocated) + continue; + ++uniform_count; if (var->is_param && var->is_uniform) @@ -1321,20 +1324,23 @@ static void write_sm1_uniforms(struct hlsl_ctx *ctx, struct vkd3d_bytecode_buffe LIST_FOR_EACH_ENTRY(var, &ctx->extern_vars, struct hlsl_ir_var, extern_entry) { - enum hlsl_regset regset = hlsl_type_get_regset(var->data_type); + unsigned int r; - if (!var->semantic.name && var->regs[regset].allocated) + for (r = 0; r <= HLSL_REGSET_LAST; ++r) { + if (var->semantic.name || !var->regs[r].allocated) + continue; + put_u32(buffer, 0); /* name */ - if (regset == HLSL_REGSET_NUMERIC) + if (r == HLSL_REGSET_NUMERIC) { - put_u32(buffer, vkd3d_make_u32(D3DXRS_FLOAT4, var->regs[regset].id)); - put_u32(buffer, var->data_type->reg_size[regset] / 4); + put_u32(buffer, vkd3d_make_u32(D3DXRS_FLOAT4, var->regs[r].id)); + put_u32(buffer, var->data_type->reg_size[r] / 4); } else { - put_u32(buffer, vkd3d_make_u32(D3DXRS_SAMPLER, var->regs[regset].id)); - put_u32(buffer, var->regs[regset].bind_count); + put_u32(buffer, vkd3d_make_u32(D3DXRS_SAMPLER, var->regs[r].id)); + put_u32(buffer, var->regs[r].bind_count); } put_u32(buffer, 0); /* type */ put_u32(buffer, 0); /* FIXME: default value */ @@ -1345,12 +1351,16 @@ static void write_sm1_uniforms(struct hlsl_ctx *ctx, struct vkd3d_bytecode_buffe LIST_FOR_EACH_ENTRY(var, &ctx->extern_vars, struct hlsl_ir_var, extern_entry) { - enum hlsl_regset regset = hlsl_type_get_regset(var->data_type); + unsigned int r; - if (!var->semantic.name && var->regs[regset].allocated) + for (r = 0; r <= HLSL_REGSET_LAST; ++r) { - size_t var_offset = vars_start + (uniform_count * 5 * sizeof(uint32_t)); - size_t name_offset; + size_t var_offset, name_offset; + + if (var->semantic.name || !var->regs[r].allocated) + continue; + + var_offset = vars_start + (uniform_count * 5 * sizeof(uint32_t)); name_offset = put_string(buffer, var->name); set_u32(buffer, var_offset, name_offset - ctab_start); -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/236
Why not using something like: ```c enum hlsl_regset regset = (var->data_type->class == HLSL_CLASS_STRUCT ? HLSL_REGSET_NUMERIC : hlsl_type_get_regset(var->data_type)); ``` possibly with a comment? It seems simpler and it better conveys the fact that variables in SM1 are still essentially considered to have a single regset. Not that your solution is bad. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/236#note_36106
On Mon Jun 19 13:25:33 2023 +0000, Giovanni Mascellani wrote:
Why not using something like: ```c enum hlsl_regset regset = (var->data_type->class == HLSL_CLASS_STRUCT ? HLSL_REGSET_NUMERIC : hlsl_type_get_regset(var->data_type)); ``` possibly with a comment? It seems simpler and it better conveys the fact that variables in SM1 are still essentially considered to have a single regset. Not that your solution is bad. I am trying to get rid of hlsl_type_get_regset() at some point, to discourage using it and having to consider exceptions such as this.
Perhaps the alternative would be making a sm1-exclusive sm1_type_get_regset() that is defined for structs, but using these `for`s also works. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/236#note_36123
On Mon Jun 19 13:25:33 2023 +0000, Francisco Casas wrote:
I am trying to get rid of hlsl_type_get_regset() at some point, to discourage using it and having to consider exceptions such as this. Perhaps the alternative would be making a sm1-exclusive sm1_type_get_regset() that is defined for structs, but using these `for`s also works. Ah, yes, it makes sense. I guess it makes sense to remove the assumption that variables only span a single regset, as much as possible.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/236#note_36241
This merge request was approved by Giovanni Mascellani. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/236
On Tue Jun 20 09:39:47 2023 +0000, Giovanni Mascellani wrote:
Ah, yes, it makes sense. I guess it makes sense to remove the assumption that variables only span a single regset, as much as possible. To be clear I haven't exactly been trying to say "hlsl_type_get_regset()" shouldn't exist. It makes me uneasy but it does (kind of) have a single definite answer for sm1.
That's not to say that the current code isn't an improvement, but at this point it probably still depends on how we actually deal with variables that have multiple register sets. From the discussion in 209 it's not really clear to me how that's shaking out. In particular, if we end up moving object fields out of structs and into their own vars, then hlsl_type_get_regset() *does* always have a single, definite answer. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/236#note_36295
This merge request was approved by Zebediah Figura. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/236
This merge request was closed by Giovanni Mascellani. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/236
Merged through !244, closing. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/236#note_36629
participants (4)
-
Francisco Casas -
Francisco Casas (＠fcasas)
-
Giovanni Mascellani (＠giomasce)
-
Zebediah Figura (＠zfigura)