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`.
From: Francisco Casas fcasas@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..b0e8015d 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 separatelly. +[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) +
From: Francisco Casas fcasas@codeweavers.com
--- tests/cbuffer.shader_test | 232 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 232 insertions(+)
diff --git a/tests/cbuffer.shader_test b/tests/cbuffer.shader_test index b0e8015d..d89ababd 100644 --- a/tests/cbuffer.shader_test +++ b/tests/cbuffer.shader_test @@ -93,3 +93,235 @@ 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] +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 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 +{ + float2 c : packoffset(c0.xz); +} + +float4 main() : sv_target +{ + return 0; +} + + +[pixel shader fail] +// packoffset cannot be used outside a constant buffer. +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; +} + + +[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) + + +[pixel shader fail] +cbuffer buffer +{ + float4 a : packoffset(c0); + Texture2D tex; +} + +float4 main() : sv_target +{ + return a + tex.Load(int3(0, 0 ,0)); +}
From: Francisco Casas fcasas@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 | 98 ++++++++++++++++++++++++++++++-- libs/vkd3d-shader/hlsl_codegen.c | 4 +- tests/cbuffer.shader_test | 26 ++++----- 6 files changed, 121 insertions(+), 24 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..4093d3a8 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; };
@@ -1079,7 +1079,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 +1114,50 @@ 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}; + + if (!sscanf(reg_string + 1, "%u", &reservation.index)) + { + 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 +2100,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 +2109,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 +3868,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 +4022,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 +4375,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 +4505,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 +4554,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 +4632,7 @@ parameter: $$.name = $3; $$.semantic = $5.semantic; $$.reg_reservation = $5.reg_reservation; + $$.offset_reservation = $5.offset_reservation; }
texture_type: @@ -4833,6 +4920,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 d89ababd..613e8b34 100644 --- a/tests/cbuffer.shader_test +++ b/tests/cbuffer.shader_test @@ -94,7 +94,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 +109,7 @@ float4 main() : sv_target }
-[pixel shader todo] +[pixel shader] // Elements can overlap if buffer is not used. cbuffer buffer { @@ -123,7 +123,7 @@ float4 main() : sv_target }
-[pixel shader todo] +[pixel shader] cbuffer buffer { float4 a : packoffset(c1); @@ -139,11 +139,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] cbuffer buffer { float2 c : packoffset(c0.y); @@ -156,11 +156,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 { @@ -173,7 +173,7 @@ float4 main() : sv_target }
-[pixel shader todo] +[pixel shader] cbuffer buffer { float4 a : packoffset(c1); @@ -190,11 +190,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 { @@ -208,7 +208,7 @@ float4 main() : sv_target }
-[pixel shader todo] +[pixel shader] cbuffer buffer { float2 c : packoffset(c0.b); @@ -221,7 +221,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)
@@ -314,7 +314,7 @@ draw quad probe all rgba (1.0, 2.0, 0.0, 4.0)
-[pixel shader fail] +[pixel shader fail todo] cbuffer buffer { float4 a : packoffset(c0);
From: Francisco Casas fcasas@codeweavers.com
--- libs/vkd3d-shader/hlsl_codegen.c | 26 ++++++++++++++++++++------ tests/cbuffer.shader_test | 10 +++++----- 2 files changed, 25 insertions(+), 11 deletions(-)
diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index aa1d2d3f..8cb91770 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -2859,17 +2859,31 @@ 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 proper_offset = hlsl_type_get_sm4_offset(var->data_type, + var->offset_reservation.index); + if (proper_offset != var->offset_reservation.index) + { + hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_RESERVATION, + "Invalid packoffset() swizzle in reservation."); + } + var->buffer_offset = proper_offset; + } + 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 +2899,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 613e8b34..5245c3e4 100644 --- a/tests/cbuffer.shader_test +++ b/tests/cbuffer.shader_test @@ -140,7 +140,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] @@ -157,10 +157,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 { @@ -191,7 +191,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] @@ -222,7 +222,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]
From: Francisco Casas fcasas@codeweavers.com
--- libs/vkd3d-shader/hlsl_codegen.c | 39 +++++++++++++++++++++----------- 1 file changed, 26 insertions(+), 13 deletions(-)
diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index 8cb91770..8afd61f1 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -2864,26 +2864,39 @@ static void calculate_buffer_offset(struct hlsl_ctx *ctx, struct hlsl_ir_var *va unsigned int var_reg_size = var->data_type->reg_size[HLSL_REGSET_NUMERIC]; struct hlsl_buffer *buffer = var->buffer;
- if (var->offset_reservation.type == 'c') + if (ctx->profile->major_version >= 4) { - unsigned int proper_offset = hlsl_type_get_sm4_offset(var->data_type, - var->offset_reservation.index); - if (proper_offset != var->offset_reservation.index) + if (var->offset_reservation.type == 'c') { - hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_RESERVATION, - "Invalid packoffset() swizzle in reservation."); + unsigned int proper_offset = hlsl_type_get_sm4_offset(var->data_type, + var->offset_reservation.index); + if (proper_offset != var->offset_reservation.index) + { + hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_RESERVATION, + "Invalid packoffset() swizzle in reservation."); + } + var->buffer_offset = proper_offset; + } + else + { + var->buffer_offset = hlsl_type_get_sm4_offset(var->data_type, buffer->size); } - var->buffer_offset = proper_offset; + + TRACE("Allocated buffer offset %u to %s.\n", var->buffer_offset, var->name); + buffer->size = max(buffer->size, var->buffer_offset + var_reg_size); + if (var->last_read) + buffer->used_size = max(buffer->used_size, var->buffer_offset + var_reg_size); } else { - var->buffer_offset = hlsl_type_get_sm4_offset(var->data_type, buffer->size); + if (var->last_read) + { + var->buffer_offset = buffer->size; + TRACE("Allocated buffer offset %u to %s.\n", var->buffer_offset, var->name); + buffer->size += var_reg_size; + buffer->used_size = buffer->size; + } } - - TRACE("Allocated buffer offset %u to %s.\n", var->buffer_offset, var->name); - buffer->size = max(buffer->size, var->buffer_offset + var_reg_size); - if (var->last_read) - buffer->used_size = max(buffer->used_size, var->buffer_offset + var_reg_size); }
static void allocate_buffers(struct hlsl_ctx *ctx)
From: Francisco Casas fcasas@codeweavers.com
--- libs/vkd3d-shader/hlsl_codegen.c | 42 ++++++++++++++++++++++++++++++++ tests/cbuffer.shader_test | 2 +- 2 files changed, 43 insertions(+), 1 deletion(-)
diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index 8afd61f1..94108766 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -2899,6 +2899,45 @@ static void calculate_buffer_offset(struct hlsl_ctx *ctx, struct hlsl_ir_var *va } }
+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; @@ -2916,6 +2955,9 @@ static void allocate_buffers(struct hlsl_ctx *ctx) } }
+ if (ctx->profile->major_version >= 4) + 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 5245c3e4..c31b5bd0 100644 --- a/tests/cbuffer.shader_test +++ b/tests/cbuffer.shader_test @@ -94,7 +94,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 {
From: Francisco Casas fcasas@codeweavers.com
--- libs/vkd3d-shader/hlsl.h | 3 +++ libs/vkd3d-shader/hlsl_codegen.c | 20 ++++++++++++++++++++ tests/cbuffer.shader_test | 4 ++-- 3 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index e1037e9a..6ae1d1a1 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; + + uint32_t manually_packed_elements : 1; + uint32_t automatically_packed_elements : 1; };
struct hlsl_ctx diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index 94108766..e25b1fa1 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -2936,6 +2936,26 @@ 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 (buffer->manually_packed_elements && buffer->automatically_packed_elements) + 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."); + } + } }
static void allocate_buffers(struct hlsl_ctx *ctx) diff --git a/tests/cbuffer.shader_test b/tests/cbuffer.shader_test index c31b5bd0..6bd0d827 100644 --- a/tests/cbuffer.shader_test +++ b/tests/cbuffer.shader_test @@ -194,7 +194,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 { @@ -314,7 +314,7 @@ draw quad probe all rgba (1.0, 2.0, 0.0, 4.0)
-[pixel shader fail todo] +[pixel shader fail] cbuffer buffer { float4 a : packoffset(c0);
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.c:
var->storage_modifiers = modifiers; if (reg_reservation) var->reg_reservation = *reg_reservation;
- if (offset_reservation)
var->offset_reservation = *offset_reservation;
Haven't thought this through all the way, but could we add another field to struct hlsl_reg_reservation?
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.y:
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.");
We don't have a test for this, right?
(Also, 8-space indentation).
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.y:
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.");
Should this be "is not allowed on functions"?
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl_codegen.c:
-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 proper_offset = hlsl_type_get_sm4_offset(var->data_type,var->offset_reservation.index);if (proper_offset != var->offset_reservation.index){hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_RESERVATION,"Invalid packoffset() swizzle in reservation.");
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.
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.
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl_codegen.c:
+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 proper_offset = hlsl_type_get_sm4_offset(var->data_type,var->offset_reservation.index);if (proper_offset != var->offset_reservation.index){hlsl_error(ctx, &var->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_RESERVATION,"Invalid packoffset() swizzle in reservation.");}var->buffer_offset = proper_offset;
This is correct, and it probably *shouldn't* matter to me that much, but semantically it makes me wince. I'd rather see "var->buffer_offset = var->offset_reservation.index".
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl_codegen.c:
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;
I guess, but if we really need to optimize, we shouldn't be doing a nested loop in the first place, so this just looks odd.
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl_codegen.c:
} }
- if (ctx->profile->major_version >= 4)
validate_buffer_offsets(ctx);
Same here, allocate_buffers() is already only called for sm4.
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl_codegen.c:
unsigned int var_reg_size = var->data_type->reg_size[HLSL_REGSET_NUMERIC]; struct hlsl_buffer *buffer = var->buffer;
- if (var->offset_reservation.type == 'c')
- if (ctx->profile->major_version >= 4)
allocate_buffers() is only called for sm4, so this entire commit should be unnecessary.
Looking at tests, here's some more tests that occur to me:
* Can the component be a "matrix" swizzle (_11 or _m00, etc.)?
* Can we test that it's legal (if ignored) to put packoffset on a resource?
* 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.
* 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?
On Tue Feb 28 00:18:59 2023 +0000, Zebediah Figura wrote:
We don't have a test for this, right? (Also, 8-space indentation).
Fixed the indentation.
Regarding the test, there is already: ``` [pixel shader fail] // packoffset cannot be used outside a constant buffer. float4 a : packoffset(c0);
float4 main() : sv_target { return 0; } ```
Regarding the test, there is already:
Oops, I should have known I was just overlooking it :-)
Giovanni Mascellani (@giomasce) commented about tests/cbuffer.shader_test:
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 separatelly.
Small typo: "separatelly"
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/hlsl.y:
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};
- if (!sscanf(reg_string + 1, "%u", &reservation.index))
This is incorrectly accepted: ``` cbuffer buffer { float4 a : packoffset(c1ha_ha_I_shouldnt_be_here_but_sscanf_just_doesnt_care); float4 b : packoffset(c2); }
float4 main() : sv_target { return 100 * a + b; } ```
On Wed Mar 1 12:26:24 2023 +0000, Zebediah Figura wrote:
Looking at tests, here's some more tests that occur to me:
- Can the component be a "matrix" swizzle (_11 or _m00, etc.)?
- Can we test that it's legal (if ignored) to put packoffset on a resource?
- 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.
- 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?
Also, what happens when you apply `packoffset()` to a nested structure, or a nested structure field?
On Tue Feb 28 19:59:13 2023 +0000, Zebediah Figura wrote:
This is correct, and it probably *shouldn't* matter to me that much, but semantically it makes me wince. I'd rather see "var->buffer_offset = var->offset_reservation.index".
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
On Tue Feb 28 00:19:00 2023 +0000, Zebediah Figura wrote:
I guess, but if we really need to optimize, we shouldn't be doing a nested loop in the first place, so this just looks odd.
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.
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/hlsl.h:
unsigned size, used_size; /* Register of type 'b' on which the buffer is allocated. */ struct hlsl_reg reg;
- uint32_t manually_packed_elements : 1;
- uint32_t automatically_packed_elements : 1;
Not a big deal, but I am not really a fan of bitfields, unless there is some compelling reason (i.e., you need that specific memory layout, or you get a significant space saving with them). However I won't object if you really want to leave them.
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/hlsl_codegen.c:
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 (buffer->manually_packed_elements && buffer->automatically_packed_elements)continue;
I guess you can throw this away if you `break` after emitting the error, can't you?
-
Francisco Casas -
Francisco Casas (＠fcasas)
-
Giovanni Mascellani (＠giomasce)
-
Zebediah Figura (＠zfigura)