[PATCH v2 0/9] MR106: vkd3d-shader/hlsl: packoffset() support.
Support manually packing constant buffer elements through the `packoffset(·)` syntax. Support not included yet for simultaneously having semantics, `register(·)`, and `packoffset(·)`, for abnormalities such as: ```hlsl Texture2D tex; cbuffer buff { float4 a : packoffset(c0); sampler sam : packoffset(c0) : register(s1) : SEMANTIC; } float4 main() : sv_target { return tex.Sample(sam, float2(0, 0)) + a; } ``` but this motivated the addition of the `hlsl_ir_var.offset_reservation` field instead of reusing `hlsl_ir_var.reg_reservation`. -- v2: vkd3d-shader/hlsl: Consider register() as manual packing for resource fields. tests: Test packoffset() with resources inside cbuffers. vkd3d-shader/hlsl: Ignore packoffset() contents for SM1. vkd3d-shader/hlsl: Don't allow manual and automatic cbuffer offset packing. vkd3d-shader/hlsl: Detect overlaps in cbuffer offsets. vkd3d-shader/hlsl: Support packoffset(). vkd3d-shader/hlsl: Parse packoffset(). tests: Test packoffset(). tests: Test cbuffer element offsets. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/106
From: Francisco Casas <fcasas(a)codeweavers.com> --- tests/cbuffer.shader_test | 78 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/tests/cbuffer.shader_test b/tests/cbuffer.shader_test index 07ef18db..d60132ea 100644 --- a/tests/cbuffer.shader_test +++ b/tests/cbuffer.shader_test @@ -15,3 +15,81 @@ float4 main() : sv_target uniform 0 float4 1.0 2.0 3.0 4.0 draw quad probe all rgba (1.0, 2.0, 3.0, 4.0) + + +% SM1 buffer offset allocation follows different rules than SM4. +% Those would have to be tested separately. +[require] +shader model >= 4.0 + + +% Respect register boundaries +[pixel shader] +cbuffer buffer +{ + float2 a; + float2 b; + float2 c; + float3 d; +} + +float4 main() : sv_target +{ + return float4(a.x, b.x, c.x, d.x); +} + +[test] +uniform 0 float4 0.0 1.0 2.0 3.0 +uniform 4 float4 4.0 5.0 6.0 7.0 +uniform 8 float4 8.0 9.0 10.0 11.0 +uniform 12 float4 12.0 13.0 14.0 15.0 +draw quad +probe all rgba (0.0, 2.0, 4.0, 8.0) + + +[pixel shader] +cbuffer buffer +{ + float a; + float b[2]; + float c; +} + +float4 main() : sv_target +{ + return float4(a, b[0], b[1], c); +} + +[test] +uniform 0 float4 0.0 1.0 2.0 3.0 +uniform 4 float4 4.0 5.0 6.0 7.0 +uniform 8 float4 8.0 9.0 10.0 11.0 +draw quad +probe all rgba (0.0, 4.0, 8.0, 9.0) + + +[pixel shader] +cbuffer buffer +{ + float a; + struct + { + float b; + float c; + } p; + float d; +} + +float4 main() : sv_target +{ + return float4(a, p.b, p.c, d); +} + +[test] +uniform 0 float4 0.0 1.0 2.0 3.0 +uniform 4 float4 4.0 5.0 6.0 7.0 +uniform 8 float4 8.0 9.0 10.0 11.0 +uniform 12 float4 12.0 13.0 14.0 15.0 +draw quad +probe all rgba (0.0, 4.0, 5.0, 6.0) + -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/106
From: Francisco Casas <fcasas(a)codeweavers.com> --- tests/cbuffer.shader_test | 295 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 295 insertions(+) diff --git a/tests/cbuffer.shader_test b/tests/cbuffer.shader_test index d60132ea..7e7beda2 100644 --- a/tests/cbuffer.shader_test +++ b/tests/cbuffer.shader_test @@ -93,3 +93,298 @@ uniform 12 float4 12.0 13.0 14.0 15.0 draw quad probe all rgba (0.0, 4.0, 5.0, 6.0) + +[pixel shader fail] +// Elements cannot overlap if buffer is used. +cbuffer buffer +{ + float c : packoffset(c0); + float4 a[2] : packoffset(c1); + float4 b[2] : packoffset(c2); +} + +float4 main() : sv_target +{ + return c; +} + + +[pixel shader todo] +// Elements can overlap if buffer is not used. +cbuffer buffer +{ + float4 a[2] : packoffset(c1); + float4 b[2] : packoffset(c2); +} + +float4 main() : sv_target +{ + return 0; +} + + +[pixel shader todo] +cbuffer buffer +{ + float4 a : packoffset(c1); + float4 b : packoffset(c2); +} + +float4 main() : sv_target +{ + return 100 * a + b; +} + +[test] +uniform 0 float4 1.0 2.0 3.0 4.0 +uniform 4 float4 5.0 6.0 7.0 8.0 +uniform 8 float4 9.0 10.0 11.0 12.0 +todo draw quad +todo probe all rgba (509, 610, 711, 812) + + +[pixel shader todo] +struct apple +{ + float2 a; + float b; + float4 c; +}; + +cbuffer buffer +{ + float4 foo : packoffset(c3); + struct apple bar : packoffset(c1); +} + +float4 main() : sv_target +{ + return 1000 * foo + 100 * float4(bar.a, 0, 0) + 10 * float4(bar.b, 0, 0, 0) + bar.c; +} + +[test] +uniform 0 float4 0.0 1.0 2.0 3.0 +uniform 4 float4 4.0 5.0 6.0 7.0 +uniform 8 float4 8.0 9.0 10.0 11.0 +uniform 12 float4 12.0 13.0 14.0 15.0 +todo draw quad +todo probe all rgba (12468.0, 13509.0, 14010.0, 15011.0) + + +[pixel shader todo] +cbuffer buffer +{ + float2 c : packoffset(c0.y); +} + +float4 main() : sv_target +{ + return float4(c, c); +} + +[test] +uniform 0 float4 1.0 2.0 3.0 4.0 +todo draw quad +todo probe all rgba (2.0, 3.0, 2.0, 3.0) + + +[pixel shader fail] +// Elements must respect register boundaries. +cbuffer buffer +{ + float4 c : packoffset(c0.z); +} + +float4 main() : sv_target +{ + return c; +} + + +[pixel shader fail] +// Invalid offset on unused buffer. +cbuffer buffer +{ + float3 a : packoffset(c0.z); +} + +float4 main() : sv_target +{ + return 0; +} + + +[pixel shader fail] +// Invalid offset on unused variable. +cbuffer buffer +{ + float a : packoffset(c0); + float3 b : packoffset(c0.z); +} + +float4 main() : sv_target +{ + return a; +} + + +[pixel shader todo] +cbuffer buffer +{ + float4 a : packoffset(c1); + float1 b : packoffset(c0); + float1 c : packoffset(c0.y); +} + +float4 main() : sv_target +{ + return 100 * a + 10 * b + c; +} + +[test] +uniform 0 float 1.0 +uniform 1 float 2.0 +uniform 4 float4 5.0 6.0 7.0 8.0 +todo draw quad +todo probe all rgba (512.0, 612.0, 712.0, 812.0) + + +[pixel shader fail] +// packoffset cannot be used unless all elements use it. +cbuffer buffer +{ + float4 a : packoffset(c0); + float4 b; +} + +float4 main() : sv_target +{ + return a + b; +} + + +[pixel shader todo] +cbuffer buffer +{ + float2 c : packoffset(c0.b); +} + +float4 main() : sv_target +{ + return float4(c, c); +} + +[test] +uniform 0 float4 1.0 2.0 3.0 4.0 +todo draw quad +todo probe all rgba (3.0, 4.0, 3.0, 4.0) + + +[pixel shader fail] +cbuffer buffer +{ + float2 c : packoffset(c0.xy); +} + +float4 main() : sv_target +{ + return 0; +} + + +[pixel shader fail] +cbuffer buffer +{ + float4x4 mat : packoffset(c0._m00); +} + +float4 main() : sv_target +{ + return 0; +} + + +[pixel shader fail] +cbuffer buffer +{ + float4 a : packoffset(c0._m00); +} + +float4 main() : sv_target +{ + return 0; +} + + +[pixel shader fail] +cbuffer buffer +{ + float2 c : packoffset(c0.xz); +} + +float4 main() : sv_target +{ + return 0; +} + + +% packoffset cannot be used outside a constant buffer. +[pixel shader fail] +float4 a : packoffset(c0); + +float4 main() : sv_target +{ + return 0; +} + +[pixel shader fail] +float4 foo(float4 a : packoffset(c0)) +{ + return a; +} + +float4 main() : sv_target +{ + return 0; +} + +[pixel shader fail] +float4 foo(float4 a) : packoffset(c0) +{ + return a; +} + +float4 main() : sv_target +{ + return 0; +} + +[pixel shader fail] +struct apple +{ + float4 a : packoffset(c0); +}; + +cbuffer buffer +{ + struct apple a; +} + +float4 main() : sv_target +{ + return 0; +} + +[pixel shader fail] +cbuffer buffer +{ + struct + { + float4 a : packoffset(c0); + } s; +} + +float4 main() : sv_target +{ + return 0; +} -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/106
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.c | 6 +- libs/vkd3d-shader/hlsl.h | 10 ++- libs/vkd3d-shader/hlsl.l | 1 + libs/vkd3d-shader/hlsl.y | 102 +++++++++++++++++++++++++++++-- libs/vkd3d-shader/hlsl_codegen.c | 4 +- tests/cbuffer.shader_test | 44 ++++++++----- 6 files changed, 140 insertions(+), 27 deletions(-) diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c index 42a1d52f..7d47feec 100644 --- a/libs/vkd3d-shader/hlsl.c +++ b/libs/vkd3d-shader/hlsl.c @@ -833,7 +833,7 @@ struct hlsl_ir_expr *hlsl_new_copy(struct hlsl_ctx *ctx, struct hlsl_ir_node *no struct hlsl_ir_var *hlsl_new_var(struct hlsl_ctx *ctx, const char *name, struct hlsl_type *type, const struct vkd3d_shader_location loc, const struct hlsl_semantic *semantic, unsigned int modifiers, - const struct hlsl_reg_reservation *reg_reservation) + const struct hlsl_reg_reservation *reg_reservation, const struct hlsl_reg_reservation *offset_reservation) { struct hlsl_ir_var *var; @@ -848,6 +848,8 @@ struct hlsl_ir_var *hlsl_new_var(struct hlsl_ctx *ctx, const char *name, struct var->storage_modifiers = modifiers; if (reg_reservation) var->reg_reservation = *reg_reservation; + if (offset_reservation) + var->offset_reservation = *offset_reservation; return var; } @@ -867,7 +869,7 @@ struct hlsl_ir_var *hlsl_new_synthetic_var(struct hlsl_ctx *ctx, const char *tem hlsl_release_string_buffer(ctx, string); return NULL; } - var = hlsl_new_var(ctx, name, type, *loc, NULL, 0, NULL); + var = hlsl_new_var(ctx, name, type, *loc, NULL, 0, NULL, NULL); hlsl_release_string_buffer(ctx, string); if (var) list_add_tail(&ctx->dummy_scope->vars, &var->scope_entry); diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index 6fa627ed..e1037e9a 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -347,7 +347,10 @@ struct hlsl_attribute #define HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT 0 /* Reservation of a specific register to a variable, field, or buffer, written in the HLSL source - * using the register(·) syntax */ + * using the register(·) syntax. + * Also used for offset reservations for objects inside constant buffers when manual packing is + * enabled through the packoffset(·) syntax. In that case, hlsl_reg_reservation.index is in + * register components. */ struct hlsl_reg_reservation { char type; @@ -367,6 +370,9 @@ struct hlsl_ir_var /* Optional register to be used as a starting point for the variable allocation, specified * by the user via the register(·) syntax. */ struct hlsl_reg_reservation reg_reservation; + /* Optional offset to be used as a starting point for variables inside constant buffers, + * specified by the user via the packoffset(·) syntax. */ + struct hlsl_reg_reservation offset_reservation; /* Item entry in hlsl_scope.vars. Specifically hlsl_ctx.globals.vars if the variable is global. */ struct list scope_entry; @@ -1073,7 +1079,7 @@ struct hlsl_ir_node *hlsl_new_unary_expr(struct hlsl_ctx *ctx, enum hlsl_ir_expr struct vkd3d_shader_location loc); struct hlsl_ir_var *hlsl_new_var(struct hlsl_ctx *ctx, const char *name, struct hlsl_type *type, const struct vkd3d_shader_location loc, const struct hlsl_semantic *semantic, unsigned int modifiers, - const struct hlsl_reg_reservation *reg_reservation); + const struct hlsl_reg_reservation *reg_reservation, const struct hlsl_reg_reservation *offset_reservation); void hlsl_error(struct hlsl_ctx *ctx, const struct vkd3d_shader_location *loc, enum vkd3d_shader_error error, const char *fmt, ...) VKD3D_PRINTF_FUNC(4, 5); diff --git a/libs/vkd3d-shader/hlsl.l b/libs/vkd3d-shader/hlsl.l index 69ed9d9d..d27fd803 100644 --- a/libs/vkd3d-shader/hlsl.l +++ b/libs/vkd3d-shader/hlsl.l @@ -95,6 +95,7 @@ matrix {return KW_MATRIX; } namespace {return KW_NAMESPACE; } nointerpolation {return KW_NOINTERPOLATION; } out {return KW_OUT; } +packoffset {return KW_PACKOFFSET; } pass {return KW_PASS; } PixelShader {return KW_PIXELSHADER; } precise {return KW_PRECISE; } diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 6c618912..12ae7735 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -39,14 +39,14 @@ struct parse_parameter struct hlsl_type *type; const char *name; struct hlsl_semantic semantic; - struct hlsl_reg_reservation reg_reservation; + struct hlsl_reg_reservation reg_reservation, offset_reservation; unsigned int modifiers; }; struct parse_colon_attribute { struct hlsl_semantic semantic; - struct hlsl_reg_reservation reg_reservation; + struct hlsl_reg_reservation reg_reservation, offset_reservation; }; struct parse_initializer @@ -71,7 +71,7 @@ struct parse_variable_def char *name; struct parse_array_sizes arrays; struct hlsl_semantic semantic; - struct hlsl_reg_reservation reg_reservation; + struct hlsl_reg_reservation reg_reservation, offset_reservation; struct parse_initializer initializer; }; @@ -997,6 +997,9 @@ static bool gen_struct_fields(struct hlsl_ctx *ctx, struct parse_fields *fields, hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_SYNTAX, "Illegal initializer on a struct field."); free_parse_initializer(&v->initializer); } + if (v->offset_reservation.type) + hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_RESERVATION, + "packoffset() is not allowed inside struct definitions."); vkd3d_free(v); } vkd3d_free(defs); @@ -1079,7 +1082,12 @@ static bool add_func_parameter(struct hlsl_ctx *ctx, struct hlsl_func_parameters hlsl_error(ctx, &loc, VKD3D_SHADER_ERROR_HLSL_INVALID_MODIFIER, "Parameter '%s' is declared as both \"out\" and \"uniform\".", param->name); - if (!(var = hlsl_new_var(ctx, param->name, param->type, loc, ¶m->semantic, param->modifiers, ¶m->reg_reservation))) + if (param->offset_reservation.type) + hlsl_error(ctx, &loc, VKD3D_SHADER_ERROR_HLSL_INVALID_RESERVATION, + "packoffset() is only allowed inside constant buffer declarations."); + + if (!(var = hlsl_new_var(ctx, param->name, param->type, loc, ¶m->semantic, param->modifiers, + ¶m->reg_reservation, NULL))) return false; var->is_param = 1; @@ -1109,6 +1117,51 @@ static struct hlsl_reg_reservation parse_reg_reservation(const char *reg_string) return reservation; } +static struct hlsl_reg_reservation parse_packoffset(struct hlsl_ctx *ctx, const char *reg_string, + const char *swizzle, const struct vkd3d_shader_location *loc) +{ + struct hlsl_reg_reservation reservation = {0}; + char invalid; + + if (sscanf(reg_string + 1, "%u%c", &reservation.index, &invalid) != 1) + { + hlsl_error(ctx, loc, VKD3D_SHADER_ERROR_HLSL_INVALID_RESERVATION, + "Invalid packoffset() syntax."); + return reservation; + } + + reservation.type = reg_string[0]; + if (reservation.type != 'c') + { + hlsl_error(ctx, loc, VKD3D_SHADER_ERROR_HLSL_INVALID_RESERVATION, + "Only 'c' registers are allowed in packoffset()."); + return reservation; + } + + reservation.index *= 4; + + if (swizzle) + { + if (strlen(swizzle) != 1) + hlsl_error(ctx, loc, VKD3D_SHADER_ERROR_HLSL_INVALID_RESERVATION, + "Invalid packoffset() swizzle \"%s\".", swizzle); + + if (swizzle[0] == 'x' || swizzle[0] == 'r') + reservation.index += 0; + else if (swizzle[0] == 'y' || swizzle[0] == 'g') + reservation.index += 1; + else if (swizzle[0] == 'z' || swizzle[0] == 'b') + reservation.index += 2; + else if (swizzle[0] == 'w' || swizzle[0] == 'a') + reservation.index += 3; + else + hlsl_error(ctx, loc, VKD3D_SHADER_ERROR_HLSL_INVALID_RESERVATION, + "Invalid packoffset() swizzle \"%s\".", swizzle); + } + + return reservation; +} + static struct hlsl_ir_function_decl *get_func_decl(struct rb_tree *funcs, const char *name, const struct hlsl_func_parameters *parameters) { @@ -2051,7 +2104,8 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t } vkd3d_free(v->arrays.sizes); - if (!(var = hlsl_new_var(ctx, v->name, type, v->loc, &v->semantic, modifiers, &v->reg_reservation))) + if (!(var = hlsl_new_var(ctx, v->name, type, v->loc, &v->semantic, modifiers, &v->reg_reservation, + &v->offset_reservation))) { free_parse_variable_def(v); continue; @@ -2059,6 +2113,13 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t var->buffer = ctx->cur_buffer; + if (var->buffer == ctx->globals_buffer) + { + if (var->offset_reservation.type) + hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_RESERVATION, + "packoffset() is only allowed inside constant buffer declarations."); + } + if (ctx->cur_scope == ctx->globals) { local = false; @@ -3811,6 +3872,7 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl %token KW_NAMESPACE %token KW_NOINTERPOLATION %token KW_OUT +%token KW_PACKOFFSET %token KW_PASS %token KW_PIXELSHADER %token KW_PRECISE @@ -3964,6 +4026,7 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl %type <parameters> parameters %type <reg_reservation> register_opt +%type <reg_reservation> packoffset_opt %type <sampler_dim> texture_type uav_type @@ -4316,6 +4379,9 @@ func_prototype_no_attrs: if ($7.reg_reservation.type) FIXME("Unexpected register reservation for a function.\n"); + if ($7.offset_reservation.type) + hlsl_error(ctx, &@5, VKD3D_SHADER_ERROR_HLSL_INVALID_RESERVATION, + "packoffset() is only allowed inside constant buffer declarations."); if (($$.decl = get_func_decl(&ctx->functions, $3, &$5))) { @@ -4443,16 +4509,25 @@ colon_attribute: { $$.semantic.name = NULL; $$.reg_reservation.type = 0; + $$.offset_reservation.type = 0; } | semantic { $$.semantic = $1; $$.reg_reservation.type = 0; + $$.offset_reservation.type = 0; } | register_opt { $$.semantic.name = NULL; $$.reg_reservation = $1; + $$.offset_reservation.type = 0; + } + | packoffset_opt + { + $$.semantic.name = NULL; + $$.reg_reservation.type = 0; + $$.offset_reservation = $1; } semantic: @@ -4483,6 +4558,21 @@ register_opt: vkd3d_free($6); } +packoffset_opt: + ':' KW_PACKOFFSET '(' any_identifier ')' + { + $$ = parse_packoffset(ctx, $4, NULL, &@$); + + vkd3d_free($4); + } + | ':' KW_PACKOFFSET '(' any_identifier '.' any_identifier ')' + { + $$ = parse_packoffset(ctx, $4, $6, &@$); + + vkd3d_free($4); + vkd3d_free($6); + } + parameters: scope_start { @@ -4546,6 +4636,7 @@ parameter: $$.name = $3; $$.semantic = $5.semantic; $$.reg_reservation = $5.reg_reservation; + $$.offset_reservation = $5.offset_reservation; } texture_type: @@ -4833,6 +4924,7 @@ variable_decl: $$->arrays = $2; $$->semantic = $3.semantic; $$->reg_reservation = $3.reg_reservation; + $$->offset_reservation = $3.offset_reservation; } state: diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index 65bd9d4c..aa1d2d3f 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -198,7 +198,7 @@ static void prepend_uniform_copy(struct hlsl_ctx *ctx, struct list *instrs, stru * can write the uniform name into the shader reflection data. */ if (!(uniform = hlsl_new_var(ctx, temp->name, temp->data_type, - temp->loc, NULL, temp->storage_modifiers, &temp->reg_reservation))) + temp->loc, NULL, temp->storage_modifiers, &temp->reg_reservation, &temp->offset_reservation))) return; list_add_before(&temp->scope_entry, &uniform->scope_entry); list_add_tail(&ctx->extern_vars, &uniform->extern_entry); @@ -238,7 +238,7 @@ static struct hlsl_ir_var *add_semantic_var(struct hlsl_ctx *ctx, struct hlsl_ir } new_semantic.index = semantic->index; if (!(ext_var = hlsl_new_var(ctx, hlsl_strdup(ctx, name->buffer), - type, var->loc, &new_semantic, modifiers, NULL))) + type, var->loc, &new_semantic, modifiers, NULL, NULL))) { hlsl_release_string_buffer(ctx, name); hlsl_cleanup_semantic(&new_semantic); diff --git a/tests/cbuffer.shader_test b/tests/cbuffer.shader_test index 7e7beda2..a40af632 100644 --- a/tests/cbuffer.shader_test +++ b/tests/cbuffer.shader_test @@ -23,6 +23,18 @@ probe all rgba (1.0, 2.0, 3.0, 4.0) shader model >= 4.0 +[pixel shader fail] +cbuffer buffer +{ + float4 a : packoffset(c1invalid_extra_chars); +} + +float4 main() : sv_target +{ + return 0; +} + + % Respect register boundaries [pixel shader] cbuffer buffer @@ -94,7 +106,7 @@ draw quad probe all rgba (0.0, 4.0, 5.0, 6.0) -[pixel shader fail] +[pixel shader fail todo] // Elements cannot overlap if buffer is used. cbuffer buffer { @@ -109,7 +121,7 @@ float4 main() : sv_target } -[pixel shader todo] +[pixel shader] // Elements can overlap if buffer is not used. cbuffer buffer { @@ -123,7 +135,7 @@ float4 main() : sv_target } -[pixel shader todo] +[pixel shader] cbuffer buffer { float4 a : packoffset(c1); @@ -139,11 +151,11 @@ float4 main() : sv_target uniform 0 float4 1.0 2.0 3.0 4.0 uniform 4 float4 5.0 6.0 7.0 8.0 uniform 8 float4 9.0 10.0 11.0 12.0 -todo draw quad +draw quad todo probe all rgba (509, 610, 711, 812) -[pixel shader todo] +[pixel shader] struct apple { float2 a; @@ -167,11 +179,11 @@ uniform 0 float4 0.0 1.0 2.0 3.0 uniform 4 float4 4.0 5.0 6.0 7.0 uniform 8 float4 8.0 9.0 10.0 11.0 uniform 12 float4 12.0 13.0 14.0 15.0 -todo draw quad +draw quad todo probe all rgba (12468.0, 13509.0, 14010.0, 15011.0) -[pixel shader todo] +[pixel shader] cbuffer buffer { float2 c : packoffset(c0.y); @@ -184,11 +196,11 @@ float4 main() : sv_target [test] uniform 0 float4 1.0 2.0 3.0 4.0 -todo draw quad +draw quad todo probe all rgba (2.0, 3.0, 2.0, 3.0) -[pixel shader fail] +[pixel shader fail todo] // Elements must respect register boundaries. cbuffer buffer { @@ -201,7 +213,7 @@ float4 main() : sv_target } -[pixel shader fail] +[pixel shader fail todo] // Invalid offset on unused buffer. cbuffer buffer { @@ -214,7 +226,7 @@ float4 main() : sv_target } -[pixel shader fail] +[pixel shader fail todo] // Invalid offset on unused variable. cbuffer buffer { @@ -228,7 +240,7 @@ float4 main() : sv_target } -[pixel shader todo] +[pixel shader] cbuffer buffer { float4 a : packoffset(c1); @@ -245,11 +257,11 @@ float4 main() : sv_target uniform 0 float 1.0 uniform 1 float 2.0 uniform 4 float4 5.0 6.0 7.0 8.0 -todo draw quad +draw quad todo probe all rgba (512.0, 612.0, 712.0, 812.0) -[pixel shader fail] +[pixel shader fail todo] // packoffset cannot be used unless all elements use it. cbuffer buffer { @@ -263,7 +275,7 @@ float4 main() : sv_target } -[pixel shader todo] +[pixel shader] cbuffer buffer { float2 c : packoffset(c0.b); @@ -276,7 +288,7 @@ float4 main() : sv_target [test] uniform 0 float4 1.0 2.0 3.0 4.0 -todo draw quad +draw quad todo probe all rgba (3.0, 4.0, 3.0, 4.0) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/106
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl_codegen.c | 27 +++++++++++++++++++++------ tests/cbuffer.shader_test | 16 ++++++++-------- 2 files changed, 29 insertions(+), 14 deletions(-) diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index aa1d2d3f..84cca40e 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -2859,17 +2859,32 @@ static const struct hlsl_buffer *get_reserved_buffer(struct hlsl_ctx *ctx, uint3 return NULL; } -static void calculate_buffer_offset(struct hlsl_ir_var *var) +static void calculate_buffer_offset(struct hlsl_ctx *ctx, struct hlsl_ir_var *var) { + unsigned int var_reg_size = var->data_type->reg_size[HLSL_REGSET_NUMERIC]; struct hlsl_buffer *buffer = var->buffer; - buffer->size = hlsl_type_get_sm4_offset(var->data_type, buffer->size); + if (var->offset_reservation.type == 'c') + { + unsigned int aligned_offset = hlsl_type_get_sm4_offset(var->data_type, + var->offset_reservation.index); + + if (aligned_offset != var->offset_reservation.index) + { + hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_RESERVATION, + "Reservation with swizzle in packoffset() breaks register boundaries."); + } + var->buffer_offset = var->offset_reservation.index; + } + else + { + var->buffer_offset = hlsl_type_get_sm4_offset(var->data_type, buffer->size); + } - var->buffer_offset = buffer->size; TRACE("Allocated buffer offset %u to %s.\n", var->buffer_offset, var->name); - buffer->size += var->data_type->reg_size[HLSL_REGSET_NUMERIC]; + buffer->size = max(buffer->size, var->buffer_offset + var_reg_size); if (var->last_read) - buffer->used_size = buffer->size; + buffer->used_size = max(buffer->used_size, var->buffer_offset + var_reg_size); } static void allocate_buffers(struct hlsl_ctx *ctx) @@ -2885,7 +2900,7 @@ static void allocate_buffers(struct hlsl_ctx *ctx) if (var->is_param) var->buffer = ctx->params_buffer; - calculate_buffer_offset(var); + calculate_buffer_offset(ctx, var); } } diff --git a/tests/cbuffer.shader_test b/tests/cbuffer.shader_test index a40af632..b8f54e77 100644 --- a/tests/cbuffer.shader_test +++ b/tests/cbuffer.shader_test @@ -152,7 +152,7 @@ uniform 0 float4 1.0 2.0 3.0 4.0 uniform 4 float4 5.0 6.0 7.0 8.0 uniform 8 float4 9.0 10.0 11.0 12.0 draw quad -todo probe all rgba (509, 610, 711, 812) +probe all rgba (509, 610, 711, 812) [pixel shader] @@ -180,7 +180,7 @@ uniform 4 float4 4.0 5.0 6.0 7.0 uniform 8 float4 8.0 9.0 10.0 11.0 uniform 12 float4 12.0 13.0 14.0 15.0 draw quad -todo probe all rgba (12468.0, 13509.0, 14010.0, 15011.0) +probe all rgba (12468.0, 13509.0, 14010.0, 15011.0) [pixel shader] @@ -197,10 +197,10 @@ float4 main() : sv_target [test] uniform 0 float4 1.0 2.0 3.0 4.0 draw quad -todo probe all rgba (2.0, 3.0, 2.0, 3.0) +probe all rgba (2.0, 3.0, 2.0, 3.0) -[pixel shader fail todo] +[pixel shader fail] // Elements must respect register boundaries. cbuffer buffer { @@ -213,7 +213,7 @@ float4 main() : sv_target } -[pixel shader fail todo] +[pixel shader fail] // Invalid offset on unused buffer. cbuffer buffer { @@ -226,7 +226,7 @@ float4 main() : sv_target } -[pixel shader fail todo] +[pixel shader fail] // Invalid offset on unused variable. cbuffer buffer { @@ -258,7 +258,7 @@ uniform 0 float 1.0 uniform 1 float 2.0 uniform 4 float4 5.0 6.0 7.0 8.0 draw quad -todo probe all rgba (512.0, 612.0, 712.0, 812.0) +probe all rgba (512.0, 612.0, 712.0, 812.0) [pixel shader fail todo] @@ -289,7 +289,7 @@ float4 main() : sv_target [test] uniform 0 float4 1.0 2.0 3.0 4.0 draw quad -todo probe all rgba (3.0, 4.0, 3.0, 4.0) +probe all rgba (3.0, 4.0, 3.0, 4.0) [pixel shader fail] -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/106
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl_codegen.c | 41 ++++++++++++++++++++++++++++++++ tests/cbuffer.shader_test | 2 +- 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index 84cca40e..04e5389b 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -2887,6 +2887,45 @@ static void calculate_buffer_offset(struct hlsl_ctx *ctx, struct hlsl_ir_var *va buffer->used_size = max(buffer->used_size, var->buffer_offset + var_reg_size); } +static void validate_buffer_offsets(struct hlsl_ctx *ctx) +{ + struct hlsl_ir_var *var1, *var2; + struct hlsl_buffer *buffer; + + LIST_FOR_EACH_ENTRY(var1, &ctx->extern_vars, struct hlsl_ir_var, extern_entry) + { + if (!var1->is_uniform || var1->data_type->type == HLSL_CLASS_OBJECT) + continue; + + buffer = var1->buffer; + if (!buffer->used_size) + continue; + + LIST_FOR_EACH_ENTRY(var2, &ctx->extern_vars, struct hlsl_ir_var, extern_entry) + { + unsigned int var1_reg_size, var2_reg_size; + + if (!var2->is_uniform || var2->data_type->type == HLSL_CLASS_OBJECT) + continue; + + if (var1 == var2 || var1->buffer != var2->buffer) + continue; + + if (strcmp(var1->name, var2->name) >= 0) + continue; + + var1_reg_size = var1->data_type->reg_size[HLSL_REGSET_NUMERIC]; + var2_reg_size = var2->data_type->reg_size[HLSL_REGSET_NUMERIC]; + + if (var1->buffer_offset < var2->buffer_offset + var2_reg_size + && var2->buffer_offset < var1->buffer_offset + var1_reg_size) + hlsl_error(ctx, &buffer->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_RESERVATION, + "Invalid packoffset() reservation: Variables %s and %s overlap.", + var1->name, var2->name); + } + } +} + static void allocate_buffers(struct hlsl_ctx *ctx) { struct hlsl_buffer *buffer; @@ -2904,6 +2943,8 @@ static void allocate_buffers(struct hlsl_ctx *ctx) } } + validate_buffer_offsets(ctx); + LIST_FOR_EACH_ENTRY(buffer, &ctx->buffers, struct hlsl_buffer, entry) { if (!buffer->used_size) diff --git a/tests/cbuffer.shader_test b/tests/cbuffer.shader_test index b8f54e77..96633cb9 100644 --- a/tests/cbuffer.shader_test +++ b/tests/cbuffer.shader_test @@ -106,7 +106,7 @@ draw quad probe all rgba (0.0, 4.0, 5.0, 6.0) -[pixel shader fail todo] +[pixel shader fail] // Elements cannot overlap if buffer is used. cbuffer buffer { -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/106
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.h | 3 +++ libs/vkd3d-shader/hlsl_codegen.c | 19 +++++++++++++++++++ tests/cbuffer.shader_test | 2 +- 3 files changed, 23 insertions(+), 1 deletion(-) diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index e1037e9a..2ba79a9e 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -684,6 +684,9 @@ struct hlsl_buffer unsigned size, used_size; /* Register of type 'b' on which the buffer is allocated. */ struct hlsl_reg reg; + + bool manually_packed_elements; + bool automatically_packed_elements; }; struct hlsl_ctx diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index 04e5389b..35f9cceb 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -2924,6 +2924,25 @@ static void validate_buffer_offsets(struct hlsl_ctx *ctx) var1->name, var2->name); } } + + LIST_FOR_EACH_ENTRY(var1, &ctx->extern_vars, struct hlsl_ir_var, extern_entry) + { + buffer = var1->buffer; + if (!buffer) + continue; + + if (var1->offset_reservation.type == 'c') + buffer->manually_packed_elements = true; + else + buffer->automatically_packed_elements = true; + + if (buffer->manually_packed_elements && buffer->automatically_packed_elements) + { + hlsl_error(ctx, &buffer->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_RESERVATION, + "packoffset() must be specified for all the buffer elements, or none of them."); + break; + } + } } static void allocate_buffers(struct hlsl_ctx *ctx) diff --git a/tests/cbuffer.shader_test b/tests/cbuffer.shader_test index 96633cb9..e7ffc27b 100644 --- a/tests/cbuffer.shader_test +++ b/tests/cbuffer.shader_test @@ -261,7 +261,7 @@ draw quad probe all rgba (512.0, 612.0, 712.0, 812.0) -[pixel shader fail todo] +[pixel shader fail] // packoffset cannot be used unless all elements use it. cbuffer buffer { -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/106
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.y | 3 +++ 1 file changed, 3 insertions(+) diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 12ae7735..ce231145 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -1123,6 +1123,9 @@ static struct hlsl_reg_reservation parse_packoffset(struct hlsl_ctx *ctx, const struct hlsl_reg_reservation reservation = {0}; char invalid; + if (ctx->profile->major_version < 4) + return reservation; + if (sscanf(reg_string + 1, "%u%c", &reservation.index, &invalid) != 1) { hlsl_error(ctx, loc, VKD3D_SHADER_ERROR_HLSL_INVALID_RESERVATION, -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/106
From: Francisco Casas <fcasas(a)codeweavers.com> --- tests/cbuffer.shader_test | 259 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 259 insertions(+) diff --git a/tests/cbuffer.shader_test b/tests/cbuffer.shader_test index e7ffc27b..6a4dda2d 100644 --- a/tests/cbuffer.shader_test +++ b/tests/cbuffer.shader_test @@ -400,3 +400,262 @@ float4 main() : sv_target { return 0; } + + +[texture 0] +size (1, 1) +0.0 0.0 0.0 4.0 + +[sampler 0] +filter linear linear linear +address clamp clamp clamp + + +[pixel shader] +// Resources are allowed inside constant buffers but they behave as regular resources. +cbuffer buffer +{ + float4 a; + Texture2D tex; + sampler sam; + float4 b; +} + +float4 main() : sv_target +{ + return a + b + tex.Sample(sam, float2(0, 0)); +} + +[test] +uniform 0 float4 1.0 0.0 0.0 0.0 +uniform 4 float4 0.0 2.0 0.0 0.0 +uniform 8 float4 0.0 0.0 3.0 0.0 +draw quad +probe all rgba (1.0, 2.0, 0.0, 4.0) + + +% packoffset() cannot be used to specify other types of registers +[pixel shader fail] +cbuffer buffer +{ + sampler sam : packoffset(s0); +} + +float4 main() : sv_target +{ + return 0; +} + +[pixel shader fail] +cbuffer buffer +{ + Texture2D tex : packoffset(t0); +} + +float4 main() : sv_target +{ + return 0; +} + + +[texture 0] +size (1, 1) +1.0 1.0 1.0 1.0 + +[texture 1] +size (1, 1) +2.0 2.0 2.0 2.0 + + +[pixel shader] +// packoffset() can be used in Textures, doesn't change the allocated t register. +cbuffer buffer +{ + Texture2D tex : packoffset(c1); +} + +float4 main() : sv_target +{ + return tex.Load(int3(0, 0, 0)); +} + +[test] +draw quad +probe all rgba (1.0, 1.0, 1.0, 1.0) + + +% Samplers cannot have packoffset(), unless register() is also specified, or they are not used. +% Note: In SM1 the rules are different: packoffset() is allowed for samplers, but they cannot be +% used together with other numeric fields, which seems like a bug. +[pixel shader fail todo] +Texture2D tex; + +cbuffer buffer +{ + sampler sam : packoffset(c1); +} + +float4 main() : sv_target +{ + return tex.Sample(sam, float2(0, 0)); +} + +[pixel shader todo] +Texture2D tex; + +cbuffer buffer +{ + sampler sam : packoffset(c1) : register(s0); +} + +float4 main() : sv_target +{ + return tex.Sample(sam, float2(0, 0)); +} + +[pixel shader] +cbuffer buffer +{ + sampler sam : packoffset(c1); +} + +float4 main() : sv_target +{ + return 0; +} + +[pixel shader] +cbuffer buffer +{ + sampler sam : packoffset(c1); + float4 a : packoffset(c0); +} + +float4 main() : sv_target +{ + return a; +} + + +% When packoffset is used in one field, resources are also expected to have a reservation. +[pixel shader fail] +cbuffer buffer +{ + float4 foo : packoffset(c0); + Texture2D tex; +} + +float4 main() : sv_target +{ + return 0; +} + +[pixel shader fail] +cbuffer buffer +{ + float4 foo : packoffset(c0); + sampler sam; +} + +float4 main() : sv_target +{ + return 0; +} + +% register() can be used instead of packoffset(). +[pixel shader todo] +cbuffer buffer +{ + float4 foo : packoffset(c0); + Texture2D tex : register(t1); +} + +float4 main() : sv_target +{ + return 0; +} + +[pixel shader todo] +cbuffer buffer +{ + float4 foo : packoffset(c0); + sampler sam : register(s1); +} + +float4 main() : sv_target +{ + return 0; +} + +% Using register() alone is considered manual packing for resources, so the other fields expect packoffset(). +[pixel shader fail todo] +cbuffer buffer +{ + float4 foo; + Texture2D tex : register(t1); +} + +float4 main() : sv_target +{ + return 0; +} + +[pixel shader fail todo] +cbuffer buffer +{ + float4 foo; + sampler sam : register(s1); +} + +float4 main() : sv_target +{ + return 0; +} + +% Using register() alone is not considered manual packing for non-resources. +[pixel shader] +cbuffer buffer +{ + float4 foo : register(c0); + Texture2D tex; +} + +float4 main() : sv_target +{ + return 0; +} + + +[require] +shader model >= 5.0 + +[texture 0] +size (1, 1) +0.0 0.0 0.0 0.5 + +[pixel shader todo] +struct apple +{ + float2 a; + Texture2D tex; + float b; +}; + +cbuffer buffer +{ + float4 foo : packoffset(c3); + struct apple bar : packoffset(c1); +} + +float4 main() : sv_target +{ + return 10 * foo + float4(bar.a, 0, 0) + float4(0, 0, bar.b, 0) + bar.tex.Load(int3(0, 0, 0)); +} + +[test] +uniform 0 float4 0.0 1.0 2.0 3.0 +uniform 4 float4 4.0 5.0 6.0 7.0 +uniform 8 float4 8.0 9.0 10.0 11.0 +uniform 12 float4 12.0 13.0 14.0 15.0 +todo draw quad +todo probe all rgba (124.0, 135.0, 146.0, 150.5) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/106
From: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl_codegen.c | 5 +++-- tests/cbuffer.shader_test | 8 ++++---- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index 35f9cceb..0c6c2375 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -2928,10 +2928,11 @@ static void validate_buffer_offsets(struct hlsl_ctx *ctx) LIST_FOR_EACH_ENTRY(var1, &ctx->extern_vars, struct hlsl_ir_var, extern_entry) { buffer = var1->buffer; - if (!buffer) + if (!buffer || buffer == ctx->globals_buffer) continue; - if (var1->offset_reservation.type == 'c') + if (var1->offset_reservation.type + || (var1->data_type->type == HLSL_CLASS_OBJECT && var1->reg_reservation.type)) buffer->manually_packed_elements = true; else buffer->automatically_packed_elements = true; diff --git a/tests/cbuffer.shader_test b/tests/cbuffer.shader_test index 6a4dda2d..da6a6cef 100644 --- a/tests/cbuffer.shader_test +++ b/tests/cbuffer.shader_test @@ -563,7 +563,7 @@ float4 main() : sv_target } % register() can be used instead of packoffset(). -[pixel shader todo] +[pixel shader] cbuffer buffer { float4 foo : packoffset(c0); @@ -575,7 +575,7 @@ float4 main() : sv_target return 0; } -[pixel shader todo] +[pixel shader] cbuffer buffer { float4 foo : packoffset(c0); @@ -588,7 +588,7 @@ float4 main() : sv_target } % Using register() alone is considered manual packing for resources, so the other fields expect packoffset(). -[pixel shader fail todo] +[pixel shader fail] cbuffer buffer { float4 foo; @@ -600,7 +600,7 @@ float4 main() : sv_target return 0; } -[pixel shader fail todo] +[pixel shader fail] cbuffer buffer { float4 foo; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/106
On Wed Mar 1 21:41:00 2023 +0000, Francisco Casas wrote:
changed this line in [version 2 of the diff](/wine/vkd3d/-/merge_requests/106/diffs?diff_id=35142&start_sha=ec0b3f4018ce8c60ce578966fcff54ae5888f5d5#9155b9453b4ec8ea0b9b025dfb55c061bd931610_4380_4387) I think that `"packoffset() is only allowed inside constant buffer declarations."` delivers more information than "packoffset() is not allowed on functions.". Native outputs a similar error, and I don't know of another circumstance where `packoffset()` can be used either.
Despite that, while addressing other comments, I realized that using `packoffsets()` inside a struct definition inside a buffer declaration is not allowed either, so that new check has a slightly more specific comment: `"packoffset() is not allowed inside struct definitions."`. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/106#note_25918
Technically true, but I feel like this could be clearer. Perhaps something like "must be properly aligned"?
"proper_offset" is not a very clear name for a variable. "aligned_offset" strikes me as clearer.
Okay, I also changed the error message to: ``` "Reservation with swizzle in packoffset() breaks register boundaries." ```
Also, should this check be here? Note that calculate_buffer_offset() is called only for variables that are used. We don't have tests for this.
Hmm, I am pretty sure that, at least now, `calculate_buffer_offset()` is called on all variables within constant buffers, not only used variables. It is worth noting that native also allocates unused variables within buffers. The only exception is when the whole buffer is not used at all, in which case it is not written (so, in the end it doesn't matter whether we allocate or not). On SM4, native throws an error on invalid offsets even if the variable (or even the whole buffer) is not used, so this behavior is correct. In SM1, native ignores packoffset and so these checks. The tests are indeed missing though, I added these two: ```hlsl [pixel shader fail] // Elements must respect register boundaries. cbuffer buffer { float4 c : packoffset(c0.z); } float4 main() : sv_target { return c; } [pixel shader fail] // Invalid offset on unused buffer. cbuffer buffer { float3 a : packoffset(c0.z); } float4 main() : sv_target { return 0; } ``` -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/106#note_25919
On Wed Mar 1 21:41:01 2023 +0000, Francisco Casas wrote:
changed this line in [version 2 of the diff](/wine/vkd3d/-/merge_requests/106/diffs?diff_id=35142&start_sha=ec0b3f4018ce8c60ce578966fcff54ae5888f5d5#3cf804f245af47d51595ff932bf817c50967eea2_2867_2867) Oh right! I didn't realize that `allocate_const_registers()` is what is taking care of that.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/106#note_25920
On Tue Feb 28 00:18:58 2023 +0000, Zebediah Figura wrote:
Haven't thought this through all the way, but could we add another field to struct hlsl_reg_reservation? I am afraid I would need clarification of what field and what for.
But if you mean reusing the same `struct hlsl_reg_reservation` value for both `register(·)` and `packoffset(·)` and adding a `bool` to distinguish, I don't think it is a good idea since, like in the example in the MR description, both can be present at the same time, even if both are not used in the end. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/106#note_25922
On Wed Mar 1 12:26:25 2023 +0000, Giovanni Mascellani wrote:
Also, doing string comparisons is at risk of being slower than what you gain. Rather, something like `if (var1 == var2) break;` should do a better job. Ah, the purpose of this check is not optimizing, but avoiding to print two error messages for each pair of overlapping variables (a, b), and (b, a).
I also thought on checking one side of the collision on each of the two iterations the pair appears. But that stills shows a double message if they completely overlap one another. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/106#note_25921
* Can the component be a "matrix" swizzle (\_11 or \_m00, etc.)?
Not according to documentation. I also just tested it in native and doesn't work. I added a couple of tests.
* Can we test that it's legal (if ignored) to put packoffset on a resource?
I added a commit with additional tests. For textures it is legal, but also forces other fields to have manual offset packing. For samplers, it can be used but a `register()` is also required at the same time (and SM1 behavior is different: packoffset() is allowed alone, but cannot be used together with other non-resource fields, which seems like a bug). For now, I think it is better to be permissive and make them follow the same less-restrictive rules of textures in this regard.
* Can we have tests that show packoffset is ignored for sm1? Also, how many of the errors implemented in these patches should apply to sm1?
Since behavior is different between SM1 and SM4, we would `[require]` something like `shader model < 4.0`, and repeat the tests. Or perhaps something like `[test sm1]` tags? I did some testing. I conclude that, in SM1: - The contents of packoffset() are subject to very little checks: `: packoffset(u.sPoNgEbOb);` compiles (but, weirdly enough, not `: packoffset(sPoNgEbOb);`. For now, I would just ignore the value and be permissive, I added a simple patch for that. - The "packoffset() is only allowed inside constant buffer declarations." still apply. - The other introduced error checks aren't checked for in SM1.
* Brief tests suggest that it's actually fine to mix packoffset numeric constants with objects that have register() but not packoffset() in the same buffer. We should probably test that and get it right; it wouldn't surprise me if that one in particular is actually used.
Oh! Good observation. I added tests and a small patch to allow this.
Also, what happens when you apply `packoffset()` to a nested structure, or a nested structure field?
Good point. I added some tests for this. It seems that I was missing throwing an error when packoffset() is used inside struct fields. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/106#note_25923
On Wed Mar 1 12:26:25 2023 +0000, Giovanni Mascellani wrote:
You're lucky in having an opinion: in these cases I'm always terribly undecided on which one to use, and waste a lot of time pondering the two (irrelevant) alternatives. https://en.wikipedia.org/wiki/Buridan%27s_ass I will give my opinion as well, then :)
In this case, I am more inclined now for `var->offset_reservation.index` since the raison d'etre of `proper_offset` is to perform the check above, and the assignment would be giving it a secondary purpose. OTOH, and the reason why I probably used `proper_offset` here, is that I got the feeling that in case of an invalid offset the program would be more stable before exiting, because otherwise the var will end with the misaligned offset provided by the user. But now `var->buffer_offset` doesn't seem to be used in a way that alters the flow of execution before the next exit point (which is before writing the sm1/sm4). Error reporting of the offset reservation overlaps could differ slightly though. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/106#note_25924
On Wed Mar 1 21:41:02 2023 +0000, Francisco Casas wrote:
changed this line in [version 2 of the diff](/wine/vkd3d/-/merge_requests/106/diffs?diff_id=35142&start_sha=ec0b3f4018ce8c60ce578966fcff54ae5888f5d5#9155b9453b4ec8ea0b9b025dfb55c061bd931610_1122_1126) oh no! :o
Thanks for pointing this out. A fun thing is that I copied that part from `parse_reg_reservation()`, and in `register()` such abominations are actually allowed. ```hlsl float4 a : register(c1ha_ha_I_shouldnt_be_here_but_sscanf_just_doesnt_care); float4 main() : sv_target { return a; } ``` I solved it for packoffset() this way, I am sure it works but I am not sure if it may be considered hacky: ```c char invalid; if (sscanf(reg_string + 1, "%u%c", &reservation.index, &invalid) != 1) { hlsl_error(ctx, loc, VKD3D_SHADER_ERROR_HLSL_INVALID_RESERVATION, "Invalid packoffset() syntax."); return reservation; } ``` -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/106#note_25925
On Wed Mar 1 21:41:03 2023 +0000, Francisco Casas wrote:
changed this line in [version 2 of the diff](/wine/vkd3d/-/merge_requests/106/diffs?diff_id=35142&start_sha=ec0b3f4018ce8c60ce578966fcff54ae5888f5d5#b4b2f0df64f7fbb515484cb602382012595ddfb6_689_688) I agree, I changed them to bool, I just wanted to be consistent with the other usages of bitfields.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/106#note_25926
On Wed Mar 1 12:26:25 2023 +0000, Giovanni Mascellani wrote:
I guess you can throw this away if you `break` after emitting the error, can't you? Ah, yes, good point. I changed that.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/106#note_25927
I solved it for packoffset() this way, I am sure it works but I am not sure if it may be considered hacky:
```c char invalid;
if (sscanf(reg_string + 1, "%u%c", &reservation.index, &invalid) != 1) ```
That works, yeah, although I'm not sure it's the most idiomatic thing. %n may be more idiomatic, or just using strtoul() instead. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/106#note_25957
On Wed Mar 1 21:42:32 2023 +0000, Francisco Casas wrote:
I am afraid I would need clarification of what field and what for. But if you mean reusing the same `struct hlsl_reg_reservation` value for both `register(·)` and `packoffset(·)` and adding a `bool` to distinguish, I don't think it is a good idea since, like in the example in the MR description, both can be present at the same time, even if both are not used in the end. No, I mean both side-by-side, e.g.
struct hlsl_reg_reservation { char reg_type; uint32_t reg_index; char packoffset_type; uint32_t packoffset_index; }; (where packoffset_type could alternatively be a boolean?) -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/106#note_25958
I think that `"packoffset() is only allowed inside constant buffer declarations."` delivers more information than `"packoffset() is not allowed on functions."`. Native outputs a similar error, and I don't know of another circumstance where `packoffset()` can be used either.
This is probably too small an issue to argue about, but I don't think I agree with this. The wording as-is has a implication of "packoffset() would be legal if this function was inside a buffer declaration" [never mind that's not actually possible]. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/106#note_25959
Technically true, but I feel like this could be clearer. Perhaps something like "must be properly aligned"?
"proper_offset" is not a very clear name for a variable. "aligned_offset" strikes me as clearer.
Okay, I also changed the error message to:
``` "Reservation with swizzle in packoffset() breaks register boundaries." ```
I'm not sure this is great either. For example, it's legal to write "float1x1 a : packoffset(c2.x)" [although, interestingly, not c2.y!] I also don't much like the term "breaks". We probably should explicitly test those two as well.
Hmm, I am pretty sure that, at least now, `calculate_buffer_offset()` is called on all variables within constant buffers, not only used variables.
Ack, you're right, I don't know how I got confused like that... -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/106#note_25960
In this case, I am more inclined now for `var->offset_reservation.index` since the raison d'etre of `proper_offset` is to perform the check above, and the assignment would be giving it a secondary purpose.
That, yeah. Not to mention that the whole point of packoffset() is to say "place the variable exactly here". There are certainly similar situations where I think it doesn't matter, though.
Error reporting of the offset reservation overlaps could differ slightly though.
Which is probably another reason we should do it this way; we don't want to report an overlap if the user didn't actually create one per se. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/106#note_25961
Ah, the purpose of this check is not optimizing, but avoiding to print two error messages for each pair of overlapping variables (a, b), and (b, a).
Ah, that makes sense. So it could probably just use a short comment. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/106#note_25962
Since behavior is different between SM1 and SM4, we would `[require]` something like `shader model < 4.0`, and repeat the tests.
Yeah, that. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/106#note_25963
On Wed Mar 1 21:42:34 2023 +0000, Francisco Casas wrote:
I agree, I changed them to bool, I just wanted to be consistent with the other usages of bitfields. I probably did because wined3d uses them a lot, but wined3d tends to have better reasons. We could probably just use bool everywhere instead.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/106#note_25964
I'm not sure this is great either. For example, it's legal to write "float1x1 a : packoffset(c2.x)" \[although, interestingly, not c2.y!\] I also don't much like the term "breaks".
The word I would find natural instead of "breaks" would be "spans". Not sure it would give the best possible error message anyway. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/106#note_25992
On Wed Mar 1 23:58:02 2023 +0000, Zebediah Figura wrote:
I solved it for packoffset() this way, I am sure it works but I am not sure if it may be considered hacky:
```c char invalid;
if (sscanf(reg_string + 1, "%u%c", &reservation.index, &invalid) != 1) ``` That works, yeah, although I'm not sure it's the most idiomatic thing. %n may be more idiomatic, or just using strtoul() instead. I'd vote for `strtoul()` (but then remember to check the end pointer!), thought any solution is fine. Honestly, even if we accepted those extra characters, I wouldn't sink in desperation.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/106#note_25993
This merge request was approved by Giovanni Mascellani. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/106
On Thu Mar 2 12:28:22 2023 +0000, Giovanni Mascellani wrote:
I'm not sure this is great either. For example, it's legal to write "float1x1 a : packoffset(c2.x)" \[although, interestingly, not c2.y!\] I also don't much like the term "breaks". The word I would find natural instead of "breaks" would be "spans". Not sure it would give the best possible error message anyway. How about:
"packoffset() reservation with swizzle does not abide to register boundaries."
? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/106#note_26020
How about:
``` "packoffset() reservation with swizzle does not abide to register boundaries." ```
?
"abide" is a somewhat unusual word here (and I don't think "abide to" is a particularly English construction either). The problem with making reference to "register boundaries" is that it's not quite the condition; types greater than vectors can and even must span multiple registers. Although I guess there's an argument that that kind of message covers all of the cases that aren't weird edge cases? Perhaps we should just have two conditions (which are probably going to be necessary in the code), along the lines of "packoffset() reservations with vector types cannot span multiple registers" and "packoffset() reservations with matrix/array/struct types cannot specify a component". [I'd also say 'component' rather than 'swizzle' honestly.] -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/106#note_26021
The wording as-is has a implication of "packoffset() would be legal if this function was inside a buffer declaration"
Hmm, I didn't consider that possible interpretation. Okay, changing the error messages in v3. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/106#note_26033
On Thu Mar 2 00:23:36 2023 +0000, Zebediah Figura wrote:
Since behavior is different between SM1 and SM4, we would `[require]` something like `shader model < 4.0`, and repeat the tests. Yeah, that. Okay, but I think these SM1 tests should be part of a different MR, since we can't specify those requirements in the shader_runner yet. Perhaps they should be in a different file too.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/106#note_26035
I'm not sure this is great either. For example, it's legal to write "float1x1 a : packoffset(c2.x)" \[although, interestingly, not c2.y!\]
You are right. I am surprised that is the case! It doesn't make much sense since automatic packing doesn't respect this restriction, e.g. : ```hlsl cbuffer buffer { float1x1 a; // packed into c0.x float1x1 b; // packed into c0.y } ``` Perhaps it is an over-generalization since larger matrices should be aligned. v2 already covers arrays and structs, but not these small matrices. Fixed it in v3.
We probably should explicitly test those two as well.
Right, I also added tests with `: packoffset (c0.y)` for structs and arrays.
Perhaps we should just have two conditions (which are probably going to be necessary in the code), along the lines of "packoffset() reservations with vector types cannot span multiple registers" and "packoffset() reservations with matrix/array/struct types cannot specify a component". \[I'd also say 'component' rather than 'swizzle' honestly.\]
Okay, I added an error message for each type. The last suggestion for error message would be a little imprecise since they can indeed specify a component, as long as it is `.x`. So I wrote: ``` "packoffset() reservations with matrix/array/struct types must be aligned with the beginning of a register." ``` in v3. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/106#note_26034
On Thu Mar 2 00:00:00 2023 +0000, Zebediah Figura wrote:
No, I mean both side-by-side, e.g. struct hlsl_reg_reservation { char reg_type; uint32_t reg_index; char packoffset_type; uint32_t packoffset_index; }; (where packoffset_type could alternatively be a boolean?) I am currently more inclined to keep them separated. Because this way we won't have to worry about "merging" two or more `struct hlsl_reg_reservations` (say, one that was returned by `parse_reg_reservation()` and other returned by `parse_packoffset()`) in the parser when we add support for parsing multiple register() and packoffset()s for the same cbuffer element.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/106#note_26036
participants (4)
-
Francisco Casas -
Francisco Casas (＠fcasas)
-
Giovanni Mascellani (＠giomasce)
-
Zebediah Figura (＠zfigura)