-- v2: vkd3d-shader/hlsl: Add functions to the global list inside the func_prototype rule. vkd3d-shader/hlsl: Use the original hlsl_ir_function_decl struct rather than allocating a new one for each definition. vkd3d-shader/hlsl: Put synthetic variables into a dummy scope. vkd3d-shader/hlsl: Add a hlsl_cleanup_semantic() helper. vkd3d-shader/hlsl: Store function parameters in an array.
From: Zebediah Figura zfigura@codeweavers.com
--- tests/hlsl-function.shader_test | 61 +++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+)
diff --git a/tests/hlsl-function.shader_test b/tests/hlsl-function.shader_test index e3ed5819..fa35963d 100644 --- a/tests/hlsl-function.shader_test +++ b/tests/hlsl-function.shader_test @@ -106,8 +106,39 @@ float4 main() : sv_target return 0; }
+% The function must have been at least declared before calling it. It may have +% been declared with a different but compatible type, though. + +[pixel shader fail] + +float4 main() : sv_target +{ + func(); + return 0; +} + +void func() +{ +} + +[pixel shader todo] + +void func(); + +float4 main() : sv_target +{ + func(); + return 0; +} + +void func() +{ +} + [pixel shader fail]
+void func(float arg); + float4 main() : sv_target { func(); @@ -120,6 +151,36 @@ void func()
[pixel shader todo]
+/* This is something of an internal test: we need to make sure that we use the + * correct variables for a function's arguments and returns regardless of + * whether it's been defined yet. + * + * Also, make sure that we can handle the case where the argument names differ. + */ + +float2 concat(float x, float y); + +float2 func(void) +{ + return concat(0.1, 0.2); +} + +float2 concat(float a, float b) +{ + return float2(a, b); +} + +float4 main() : sv_target +{ + return float4(func(), concat(0.3, 0.4)); +} + +[test] +todo draw quad +todo probe all rgba (0.1, 0.2, 0.3, 0.4) + +[pixel shader todo] + float func(in float a, out float b, inout float c) { c -= 0.2;
From: Zebediah Figura zfigura@codeweavers.com
--- libs/vkd3d-shader/hlsl.c | 50 ++++++++++++-------------- libs/vkd3d-shader/hlsl.h | 19 ++++++---- libs/vkd3d-shader/hlsl.y | 62 +++++++++++++++++--------------- libs/vkd3d-shader/hlsl_codegen.c | 4 ++- 4 files changed, 70 insertions(+), 65 deletions(-)
diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c index 256e466a..f62057b8 100644 --- a/libs/vkd3d-shader/hlsl.c +++ b/libs/vkd3d-shader/hlsl.c @@ -1200,8 +1200,9 @@ struct hlsl_ir_loop *hlsl_new_loop(struct hlsl_ctx *ctx, struct vkd3d_shader_loc return loop; }
-struct hlsl_ir_function_decl *hlsl_new_func_decl(struct hlsl_ctx *ctx, struct hlsl_type *return_type, - struct list *parameters, const struct hlsl_semantic *semantic, const struct vkd3d_shader_location *loc) +struct hlsl_ir_function_decl *hlsl_new_func_decl(struct hlsl_ctx *ctx, + struct hlsl_type *return_type, const struct hlsl_func_parameters *parameters, + const struct hlsl_semantic *semantic, const struct vkd3d_shader_location *loc) { struct hlsl_ir_function_decl *decl;
@@ -1209,7 +1210,7 @@ struct hlsl_ir_function_decl *hlsl_new_func_decl(struct hlsl_ctx *ctx, struct hl return NULL; list_init(&decl->body.instrs); decl->return_type = return_type; - decl->parameters = parameters; + decl->parameters = *parameters; decl->loc = *loc;
if (!hlsl_types_are_equal(return_type, ctx->builtin_types.Void)) @@ -1336,27 +1337,18 @@ static int compare_param_hlsl_types(const struct hlsl_type *t1, const struct hls
static int compare_function_decl_rb(const void *key, const struct rb_entry *entry) { - const struct list *params = key; const struct hlsl_ir_function_decl *decl = RB_ENTRY_VALUE(entry, const struct hlsl_ir_function_decl, entry); - int decl_params_count = decl->parameters ? list_count(decl->parameters) : 0; - int params_count = params ? list_count(params) : 0; - struct list *p1cur, *p2cur; + const struct hlsl_func_parameters *parameters = key; + size_t i; int r;
- if ((r = vkd3d_u32_compare(params_count, decl_params_count))) + if ((r = vkd3d_u32_compare(parameters->count, decl->parameters.count))) return r;
- p1cur = params ? list_head(params) : NULL; - p2cur = decl->parameters ? list_head(decl->parameters) : NULL; - while (p1cur && p2cur) + for (i = 0; i < parameters->count; ++i) { - struct hlsl_ir_var *p1, *p2; - p1 = LIST_ENTRY(p1cur, struct hlsl_ir_var, param_entry); - p2 = LIST_ENTRY(p2cur, struct hlsl_ir_var, param_entry); - if ((r = compare_param_hlsl_types(p1->data_type, p2->data_type))) + if ((r = compare_param_hlsl_types(parameters->vars[i]->data_type, decl->parameters.vars[i]->data_type))) return r; - p1cur = list_next(params, p1cur); - p2cur = list_next(decl->parameters, p2cur); } return 0; } @@ -1655,7 +1647,7 @@ static void dump_ir_call(struct hlsl_ctx *ctx, struct vkd3d_string_buffer *buffe { const struct hlsl_ir_function_decl *decl = call->decl; struct vkd3d_string_buffer *string; - const struct hlsl_ir_var *param; + size_t i;
if (!(string = hlsl_type_to_string(ctx, decl->return_type))) return; @@ -1663,14 +1655,16 @@ static void dump_ir_call(struct hlsl_ctx *ctx, struct vkd3d_string_buffer *buffe vkd3d_string_buffer_printf(buffer, "call %s %s(", string->buffer, decl->func->name); hlsl_release_string_buffer(ctx, string);
- LIST_FOR_EACH_ENTRY(param, decl->parameters, struct hlsl_ir_var, param_entry) + for (i = 0; i < decl->parameters.count; ++i) { + const struct hlsl_ir_var *param = decl->parameters.vars[i]; + if (!(string = hlsl_type_to_string(ctx, param->data_type))) return;
- vkd3d_string_buffer_printf(buffer, "%s", string->buffer); - if (list_tail(decl->parameters) != ¶m->param_entry) + if (i) vkd3d_string_buffer_printf(buffer, ", "); + vkd3d_string_buffer_printf(buffer, "%s", string->buffer);
hlsl_release_string_buffer(ctx, string); } @@ -1959,14 +1953,14 @@ static void dump_instr(struct hlsl_ctx *ctx, struct vkd3d_string_buffer *buffer, void hlsl_dump_function(struct hlsl_ctx *ctx, const struct hlsl_ir_function_decl *func) { struct vkd3d_string_buffer buffer; - struct hlsl_ir_var *param; + size_t i;
vkd3d_string_buffer_init(&buffer); vkd3d_string_buffer_printf(&buffer, "Dumping function %s.\n", func->func->name); vkd3d_string_buffer_printf(&buffer, "Function parameters:\n"); - LIST_FOR_EACH_ENTRY(param, func->parameters, struct hlsl_ir_var, param_entry) + for (i = 0; i < func->parameters.count; ++i) { - dump_ir_var(ctx, &buffer, param); + dump_ir_var(ctx, &buffer, func->parameters.vars[i]); vkd3d_string_buffer_printf(&buffer, "\n"); } if (func->has_body) @@ -2168,7 +2162,7 @@ static void free_function_decl(struct hlsl_ir_function_decl *decl) hlsl_free_attribute((void *)decl->attrs[i]); vkd3d_free((void *)decl->attrs);
- vkd3d_free(decl->parameters); + vkd3d_free(decl->parameters.vars); hlsl_free_instr_list(&decl->body.instrs); vkd3d_free(decl); } @@ -2200,7 +2194,7 @@ void hlsl_add_function(struct hlsl_ctx *ctx, char *name, struct hlsl_ir_function { func = RB_ENTRY_VALUE(func_entry, struct hlsl_ir_function, entry); decl->func = func; - if ((old_entry = rb_get(&func->overloads, decl->parameters))) + if ((old_entry = rb_get(&func->overloads, &decl->parameters))) { struct hlsl_ir_function_decl *old_decl = RB_ENTRY_VALUE(old_entry, struct hlsl_ir_function_decl, entry); @@ -2224,7 +2218,7 @@ void hlsl_add_function(struct hlsl_ctx *ctx, char *name, struct hlsl_ir_function rb_remove(&func->overloads, old_entry); free_function_decl(old_decl); } - rb_put(&func->overloads, decl->parameters, &decl->entry); + rb_put(&func->overloads, &decl->parameters, &decl->entry); vkd3d_free(name); return; } @@ -2232,7 +2226,7 @@ void hlsl_add_function(struct hlsl_ctx *ctx, char *name, struct hlsl_ir_function func->name = name; rb_init(&func->overloads, compare_function_decl_rb); decl->func = func; - rb_put(&func->overloads, decl->parameters, &decl->entry); + rb_put(&func->overloads, &decl->parameters, &decl->entry); rb_put(&ctx->functions, func->name, &func->entry); }
diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index bb63f827..a3fd1309 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -360,8 +360,6 @@ struct hlsl_ir_var
/* Item entry in hlsl_scope.vars. Specifically hlsl_ctx.globals.vars if the variable is global. */ struct list scope_entry; - /* Item entry in hlsl_ir_function_decl.parameters, if the variable is a function parameter. */ - struct list param_entry; /* Item entry in hlsl_ctx.extern_vars, if the variable is extern. */ struct list extern_entry;
@@ -392,6 +390,13 @@ struct hlsl_ir_var uint32_t is_param : 1; };
+/* Sized array of variables representing a function's parameters. */ +struct hlsl_func_parameters +{ + struct hlsl_ir_var **vars; + size_t count, capacity; +}; + struct hlsl_ir_function { /* Item entry in hlsl_ctx.functions */ @@ -415,9 +420,8 @@ struct hlsl_ir_function_decl
/* Function to which this declaration corresponds. */ struct hlsl_ir_function *func; - /* List containing one variable for each parameter of the function; linked by the - * hlsl_ir_var.param_entry fields. */ - struct list *parameters; + + struct hlsl_func_parameters parameters;
struct hlsl_block body; bool has_body; @@ -1005,8 +1009,9 @@ struct hlsl_ir_node *hlsl_new_expr(struct hlsl_ctx *ctx, enum hlsl_ir_expr_op op struct hlsl_type *data_type, const struct vkd3d_shader_location *loc); struct hlsl_ir_constant *hlsl_new_float_constant(struct hlsl_ctx *ctx, float f, const struct vkd3d_shader_location *loc); -struct hlsl_ir_function_decl *hlsl_new_func_decl(struct hlsl_ctx *ctx, struct hlsl_type *return_type, - struct list *parameters, const struct hlsl_semantic *semantic, const struct vkd3d_shader_location *loc); +struct hlsl_ir_function_decl *hlsl_new_func_decl(struct hlsl_ctx *ctx, + struct hlsl_type *return_type, const struct hlsl_func_parameters *parameters, + const struct hlsl_semantic *semantic, const struct vkd3d_shader_location *loc); struct hlsl_ir_if *hlsl_new_if(struct hlsl_ctx *ctx, struct hlsl_ir_node *condition, struct vkd3d_shader_location loc); struct hlsl_ir_constant *hlsl_new_int_constant(struct hlsl_ctx *ctx, int n, const struct vkd3d_shader_location *loc); diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index a4de0edd..00af3f46 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -1067,7 +1067,7 @@ static bool add_typedef(struct hlsl_ctx *ctx, DWORD modifiers, struct hlsl_type return true; }
-static bool add_func_parameter(struct hlsl_ctx *ctx, struct list *list, +static bool add_func_parameter(struct hlsl_ctx *ctx, struct hlsl_func_parameters *parameters, struct parse_parameter *param, const struct vkd3d_shader_location loc) { struct hlsl_ir_var *var; @@ -1088,7 +1088,11 @@ static bool add_func_parameter(struct hlsl_ctx *ctx, struct list *list, hlsl_free_var(var); return false; } - list_add_tail(list, &var->param_entry); + + if (!hlsl_array_reserve(ctx, (void **)¶meters->vars, ¶meters->capacity, + parameters->count + 1, sizeof(*parameters->vars))) + return false; + parameters->vars[parameters->count++] = var; return true; }
@@ -1105,7 +1109,8 @@ static struct hlsl_reg_reservation parse_reg_reservation(const char *reg_string) return reservation; }
-static const struct hlsl_ir_function_decl *get_func_decl(struct rb_tree *funcs, char *name, struct list *params) +static const struct hlsl_ir_function_decl *get_func_decl(struct rb_tree *funcs, + const char *name, const struct hlsl_func_parameters *parameters) { struct hlsl_ir_function *func; struct rb_entry *entry; @@ -1114,7 +1119,7 @@ static const struct hlsl_ir_function_decl *get_func_decl(struct rb_tree *funcs, { func = RB_ENTRY_VALUE(entry, struct hlsl_ir_function, entry);
- if ((entry = rb_get(&func->overloads, params))) + if ((entry = rb_get(&func->overloads, parameters))) return RB_ENTRY_VALUE(entry, struct hlsl_ir_function_decl, entry); } return NULL; @@ -2224,17 +2229,17 @@ static void find_function_call_exact(struct rb_entry *entry, void *context) { struct hlsl_ir_function_decl *decl = RB_ENTRY_VALUE(entry, struct hlsl_ir_function_decl, entry); struct find_function_call_args *args = context; - const struct hlsl_ir_var *param; - unsigned int i = 0; + unsigned int i;
- LIST_FOR_EACH_ENTRY(param, decl->parameters, struct hlsl_ir_var, param_entry) + if (decl->parameters.count != args->params->args_count) + return; + + for (i = 0; i < decl->parameters.count; ++i) { - if (i >= args->params->args_count - || !hlsl_types_are_equal(param->data_type, args->params->args[i++]->data_type)) + if (!hlsl_types_are_equal(decl->parameters.vars[i]->data_type, args->params->args[i]->data_type)) return; } - if (i == args->params->args_count) - args->decl = decl; + args->decl = decl; }
static struct hlsl_ir_function_decl *find_function_call(struct hlsl_ctx *ctx, @@ -3000,15 +3005,14 @@ static struct list *add_call(struct hlsl_ctx *ctx, const char *name, if ((decl = find_function_call(ctx, name, args))) { struct hlsl_ir_node *call; - struct hlsl_ir_var *param; unsigned int i;
- assert(args->args_count == list_count(decl->parameters)); + assert(args->args_count == decl->parameters.count);
- i = 0; - LIST_FOR_EACH_ENTRY(param, decl->parameters, struct hlsl_ir_var, param_entry) + for (i = 0; i < decl->parameters.count; ++i) { - struct hlsl_ir_node *arg = args->args[i++]; + struct hlsl_ir_var *param = decl->parameters.vars[i]; + struct hlsl_ir_node *arg = args->args[i];
if (!hlsl_types_are_equal(arg->data_type, param->data_type)) { @@ -3030,10 +3034,10 @@ static struct list *add_call(struct hlsl_ctx *ctx, const char *name, goto fail; list_add_tail(args->instrs, &call->entry);
- i = 0; - LIST_FOR_EACH_ENTRY(param, decl->parameters, struct hlsl_ir_var, param_entry) + for (i = 0; i < decl->parameters.count; ++i) { - struct hlsl_ir_node *arg = args->args[i++]; + struct hlsl_ir_var *param = decl->parameters.vars[i]; + struct hlsl_ir_node *arg = args->args[i];
if (param->storage_modifiers & HLSL_STORAGE_OUT) { @@ -3510,6 +3514,7 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl struct parse_fields fields; struct parse_function function; struct parse_parameter parameter; + struct hlsl_func_parameters parameters; struct parse_initializer initializer; struct parse_array_sizes arrays; struct parse_variable_def *variable_def; @@ -3647,8 +3652,6 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl %type <list> logicor_expr %type <list> loop_statement %type <list> mul_expr -%type <list> param_list -%type <list> parameters %type <list> postfix_expr %type <list> primary_expr %type <list> relational_expr @@ -3702,6 +3705,9 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl
%type <parameter> parameter
+%type <parameters> param_list +%type <parameters> parameters + %type <reg_reservation> register_opt
%type <sampler_dim> texture_type uav_type @@ -3727,7 +3733,7 @@ hlsl_prog: { const struct hlsl_ir_function_decl *decl;
- decl = get_func_decl(&ctx->functions, $2.name, $2.decl->parameters); + decl = get_func_decl(&ctx->functions, $2.name, &$2.decl->parameters); if (decl) { if (decl->has_body && $2.decl->has_body) @@ -4032,7 +4038,7 @@ func_prototype_no_attrs: if ($7.reg_reservation.type) FIXME("Unexpected register reservation for a function.\n");
- if (!($$.decl = hlsl_new_func_decl(ctx, type, $5, &$7.semantic, &@3))) + if (!($$.decl = hlsl_new_func_decl(ctx, type, &$5, &$7.semantic, &@3))) YYABORT; $$.name = $3; ctx->cur_function = $$.decl; @@ -4117,8 +4123,7 @@ register_opt: parameters: scope_start { - if (!($$ = make_empty_list(ctx))) - YYABORT; + memset(&$$, 0, sizeof($$)); } | scope_start param_list { @@ -4128,9 +4133,8 @@ parameters: param_list: parameter { - if (!($$ = make_empty_list(ctx))) - YYABORT; - if (!add_func_parameter(ctx, $$, &$1, @1)) + memset(&$$, 0, sizeof($$)); + if (!add_func_parameter(ctx, &$$, &$1, @1)) { ERR("Error adding function parameter %s.\n", $1.name); YYABORT; @@ -4139,7 +4143,7 @@ param_list: | param_list ',' parameter { $$ = $1; - if (!add_func_parameter(ctx, $$, &$3, @3)) + if (!add_func_parameter(ctx, &$$, &$3, @3)) { hlsl_error(ctx, &@3, VKD3D_SHADER_ERROR_HLSL_REDEFINED, "Parameter "%s" is already declared.", $3.name); diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index 4fa860a6..f6f096f1 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -2877,8 +2877,10 @@ int hlsl_emit_bytecode(struct hlsl_ctx *ctx, struct hlsl_ir_function_decl *entry prepend_uniform_copy(ctx, &body->instrs, var); }
- LIST_FOR_EACH_ENTRY(var, entry_func->parameters, struct hlsl_ir_var, param_entry) + for (i = 0; i < entry_func->parameters.count; ++i) { + var = entry_func->parameters.vars[i]; + if (var->data_type->type == HLSL_CLASS_OBJECT || (var->storage_modifiers & HLSL_STORAGE_UNIFORM)) { prepend_uniform_copy(ctx, &body->instrs, var);
From: Zebediah Figura zfigura@codeweavers.com
--- libs/vkd3d-shader/hlsl.c | 10 ++++++++-- libs/vkd3d-shader/hlsl.h | 2 ++ libs/vkd3d-shader/hlsl.y | 2 +- libs/vkd3d-shader/hlsl_codegen.c | 2 +- 4 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c index f62057b8..3bc0ac3c 100644 --- a/libs/vkd3d-shader/hlsl.c +++ b/libs/vkd3d-shader/hlsl.c @@ -113,7 +113,7 @@ struct hlsl_ir_var *hlsl_get_var(struct hlsl_scope *scope, const char *name) void hlsl_free_var(struct hlsl_ir_var *decl) { vkd3d_free((void *)decl->name); - vkd3d_free((void *)decl->semantic.name); + hlsl_cleanup_semantic(&decl->semantic); vkd3d_free(decl); }
@@ -1997,7 +1997,7 @@ void hlsl_free_type(struct hlsl_type *type) field = &type->e.record.fields[i];
vkd3d_free((void *)field->name); - vkd3d_free((void *)field->semantic.name); + hlsl_cleanup_semantic(&field->semantic); } vkd3d_free((void *)type->e.record.fields); } @@ -2154,6 +2154,12 @@ void hlsl_free_attribute(struct hlsl_attribute *attr) vkd3d_free(attr); }
+void hlsl_cleanup_semantic(struct hlsl_semantic *semantic) +{ + vkd3d_free((void *)semantic->name); + memset(semantic, 0, sizeof(*semantic)); +} + static void free_function_decl(struct hlsl_ir_function_decl *decl) { unsigned int i; diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index a3fd1309..53ac3363 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -973,7 +973,9 @@ int hlsl_emit_bytecode(struct hlsl_ctx *ctx, struct hlsl_ir_function_decl *entry enum vkd3d_shader_target_type target_type, struct vkd3d_shader_code *out);
bool hlsl_copy_deref(struct hlsl_ctx *ctx, struct hlsl_deref *deref, const struct hlsl_deref *other); + void hlsl_cleanup_deref(struct hlsl_deref *deref); +void hlsl_cleanup_semantic(struct hlsl_semantic *semantic);
void hlsl_replace_node(struct hlsl_ir_node *old, struct hlsl_ir_node *new);
diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 00af3f46..a0e2707f 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -922,7 +922,7 @@ static void free_parse_variable_def(struct parse_variable_def *v) free_parse_initializer(&v->initializer); vkd3d_free(v->arrays.sizes); vkd3d_free(v->name); - vkd3d_free((void *)v->semantic.name); + hlsl_cleanup_semantic(&v->semantic); vkd3d_free(v); }
diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index f6f096f1..d767c4da 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -227,7 +227,7 @@ static struct hlsl_ir_var *add_semantic_var(struct hlsl_ctx *ctx, struct hlsl_ir type, var->loc, &new_semantic, modifiers, NULL))) { hlsl_release_string_buffer(ctx, name); - vkd3d_free((void *)new_semantic.name); + hlsl_cleanup_semantic(&new_semantic); return NULL; } hlsl_release_string_buffer(ctx, name);
From: Zebediah Figura zfigura@codeweavers.com
Prevent them from being ever looked up.
Our naming scheme for synthetic variables already effectively prevents this, but this is better for clarity. We also will need to be able to move some named variables into a dummy scope to account for complexities around function definition and declarations. --- libs/vkd3d-shader/hlsl.c | 30 ++++++++++++++++++++++++------ libs/vkd3d-shader/hlsl.h | 2 ++ 2 files changed, 26 insertions(+), 6 deletions(-)
diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c index 3bc0ac3c..9fde78c2 100644 --- a/libs/vkd3d-shader/hlsl.c +++ b/libs/vkd3d-shader/hlsl.c @@ -804,7 +804,7 @@ struct hlsl_ir_var *hlsl_new_synthetic_var(struct hlsl_ctx *ctx, const char *tem var = hlsl_new_var(ctx, name, type, *loc, NULL, 0, NULL); hlsl_release_string_buffer(ctx, string); if (var) - list_add_tail(&ctx->globals->vars, &var->scope_entry); + list_add_tail(&ctx->dummy_scope->vars, &var->scope_entry); return var; }
@@ -1258,18 +1258,29 @@ static int compare_hlsl_types_rb(const void *key, const struct rb_entry *entry) return strcmp(name, type->name); }
+static struct hlsl_scope *hlsl_new_scope(struct hlsl_ctx *ctx, struct hlsl_scope *upper) +{ + struct hlsl_scope *scope; + + if (!(scope = hlsl_alloc(ctx, sizeof(*scope)))) + return NULL; + TRACE("Pushing a new scope.\n"); + list_init(&scope->vars); + rb_init(&scope->types, compare_hlsl_types_rb); + scope->upper = upper; + list_add_tail(&ctx->scopes, &scope->entry); + return scope; +} + void hlsl_push_scope(struct hlsl_ctx *ctx) { struct hlsl_scope *new_scope;
- if (!(new_scope = hlsl_alloc(ctx, sizeof(*new_scope)))) + if (!(new_scope = hlsl_new_scope(ctx, ctx->cur_scope))) return; + TRACE("Pushing a new scope.\n"); - list_init(&new_scope->vars); - rb_init(&new_scope->types, compare_hlsl_types_rb); - new_scope->upper = ctx->cur_scope; ctx->cur_scope = new_scope; - list_add_tail(&ctx->scopes, &new_scope->entry); }
void hlsl_pop_scope(struct hlsl_ctx *ctx) @@ -2571,6 +2582,13 @@ static bool hlsl_ctx_init(struct hlsl_ctx *ctx, const char *source_name, ctx->matrix_majority = HLSL_COLUMN_MAJOR;
list_init(&ctx->scopes); + + if (!(ctx->dummy_scope = hlsl_new_scope(ctx, NULL))) + { + vkd3d_free((void *)ctx->source_files[0]); + vkd3d_free(ctx->source_files); + return false; + } hlsl_push_scope(ctx); ctx->globals = ctx->cur_scope;
diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index 53ac3363..b2e36e7c 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -697,6 +697,8 @@ struct hlsl_ctx struct hlsl_scope *cur_scope; /* Scope of global variables. */ struct hlsl_scope *globals; + /* Dummy scope for variables which should never be looked up by name. */ + struct hlsl_scope *dummy_scope; /* List of all the scopes in the program; linked by the hlsl_scope.entry fields. */ struct list scopes; /* List of all the extern variables; linked by the hlsl_ir_var.extern_entry fields.
From: Zebediah Figura zfigura@codeweavers.com
We need to make sure every invocation points to the same hlsl_ir_function_decl and the same parameters.
This fixes some invalid memory accesses. --- libs/vkd3d-shader/hlsl.c | 28 +----- libs/vkd3d-shader/hlsl.y | 178 ++++++++++++++++++++++++++++++++------- 2 files changed, 150 insertions(+), 56 deletions(-)
diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c index 9fde78c2..c06ed7de 100644 --- a/libs/vkd3d-shader/hlsl.c +++ b/libs/vkd3d-shader/hlsl.c @@ -2204,38 +2204,16 @@ static void free_function_rb(struct rb_entry *entry, void *context) void hlsl_add_function(struct hlsl_ctx *ctx, char *name, struct hlsl_ir_function_decl *decl) { struct hlsl_ir_function *func; - struct rb_entry *func_entry, *old_entry; + struct rb_entry *func_entry;
func_entry = rb_get(&ctx->functions, name); if (func_entry) { func = RB_ENTRY_VALUE(func_entry, struct hlsl_ir_function, entry); decl->func = func; - if ((old_entry = rb_get(&func->overloads, &decl->parameters))) - { - struct hlsl_ir_function_decl *old_decl = - RB_ENTRY_VALUE(old_entry, struct hlsl_ir_function_decl, entry); - unsigned int i;
- if (!decl->has_body) - { - free_function_decl(decl); - vkd3d_free(name); - return; - } - - for (i = 0; i < decl->attr_count; ++i) - hlsl_free_attribute((void *)decl->attrs[i]); - vkd3d_free((void *)decl->attrs); - decl->attr_count = old_decl->attr_count; - decl->attrs = old_decl->attrs; - old_decl->attr_count = 0; - old_decl->attrs = NULL; - - rb_remove(&func->overloads, old_entry); - free_function_decl(old_decl); - } - rb_put(&func->overloads, &decl->parameters, &decl->entry); + if (rb_put(&func->overloads, &decl->parameters, &decl->entry) == -1) + ERR("Failed to insert function overload.\n"); vkd3d_free(name); return; } diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index a0e2707f..8207daba 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -79,6 +79,9 @@ struct parse_function { char *name; struct hlsl_ir_function_decl *decl; + struct hlsl_func_parameters parameters; + struct hlsl_semantic return_semantic; + bool first; };
struct parse_if_body @@ -1109,7 +1112,7 @@ static struct hlsl_reg_reservation parse_reg_reservation(const char *reg_string) return reservation; }
-static const struct hlsl_ir_function_decl *get_func_decl(struct rb_tree *funcs, +static struct hlsl_ir_function_decl *get_func_decl(struct rb_tree *funcs, const char *name, const struct hlsl_func_parameters *parameters) { struct hlsl_ir_function *func; @@ -3731,28 +3734,15 @@ hlsl_prog: %empty | hlsl_prog func_declaration { - const struct hlsl_ir_function_decl *decl; - - decl = get_func_decl(&ctx->functions, $2.name, &$2.decl->parameters); - if (decl) + if ($2.first) { - if (decl->has_body && $2.decl->has_body) - { - hlsl_error(ctx, &$2.decl->loc, VKD3D_SHADER_ERROR_HLSL_REDEFINED, - "Function "%s" is already defined.", $2.name); - hlsl_note(ctx, &decl->loc, VKD3D_SHADER_LOG_ERROR, ""%s" was previously defined here.", $2.name); - YYABORT; - } - else if (!hlsl_types_are_equal(decl->return_type, $2.decl->return_type)) - { - hlsl_error(ctx, &$2.decl->loc, VKD3D_SHADER_ERROR_HLSL_REDEFINED, - "Function "%s" was already declared with a different return type.", $2.name); - hlsl_note(ctx, &decl->loc, VKD3D_SHADER_LOG_ERROR, ""%s" was previously declared here.", $2.name); - YYABORT; - } + hlsl_add_function(ctx, $2.name, $2.decl); + } + else + { + vkd3d_free($2.parameters.vars); + hlsl_cleanup_semantic(&$2.return_semantic); } - - hlsl_add_function(ctx, $2.name, $2.decl); } | hlsl_prog buffer_declaration buffer_body | hlsl_prog declaration_statement @@ -3997,11 +3987,55 @@ attribute_list: func_declaration: func_prototype compound_statement { - $$ = $1; - $$.decl->has_body = true; - list_move_tail(&$$.decl->body.instrs, $2); - vkd3d_free($2); + struct hlsl_ir_function_decl *decl = $1.decl; + + if (decl->has_body) + { + hlsl_error(ctx, &decl->loc, VKD3D_SHADER_ERROR_HLSL_REDEFINED, + "Function "%s" is already defined.", decl->func->name); + hlsl_note(ctx, &decl->loc, VKD3D_SHADER_LOG_ERROR, + ""%s" was previously defined here.", decl->func->name); + hlsl_free_instr_list($2); + } + else + { + size_t i; + + decl->has_body = true; + list_move_tail(&decl->body.instrs, $2); + vkd3d_free($2); + + /* Semantics are taken from whichever definition has a body. + * We can't just replace the hlsl_ir_var pointers, though: if + * the function was already declared but not defined, the + * callers would have used the old declaration's parameters to + * transfer arguments. */ + + if (!$1.first) + { + assert(decl->parameters.count == $1.parameters.count); + + for (i = 0; i < $1.parameters.count; ++i) + { + struct hlsl_ir_var *dst = decl->parameters.vars[i]; + struct hlsl_ir_var *src = $1.parameters.vars[i]; + + hlsl_cleanup_semantic(&dst->semantic); + dst->semantic = src->semantic; + memset(&src->semantic, 0, sizeof(src->semantic)); + } + + if (decl->return_var) + { + hlsl_cleanup_semantic(&decl->return_var->semantic); + decl->return_var->semantic = $1.return_semantic; + memset(&$1.return_semantic, 0, sizeof($1.return_semantic)); + } + } + } hlsl_pop_scope(ctx); + + $$ = $1; } | func_prototype ';' { @@ -4038,18 +4072,100 @@ func_prototype_no_attrs: if ($7.reg_reservation.type) FIXME("Unexpected register reservation for a function.\n");
- if (!($$.decl = hlsl_new_func_decl(ctx, type, &$5, &$7.semantic, &@3))) - YYABORT; - $$.name = $3; - ctx->cur_function = $$.decl; + if (($$.decl = get_func_decl(&ctx->functions, $3, &$5))) + { + const struct hlsl_func_parameters *params = &$$.decl->parameters; + size_t i; + + if (!hlsl_types_are_equal($2, $$.decl->return_type)) + { + hlsl_error(ctx, &@3, VKD3D_SHADER_ERROR_HLSL_REDEFINED, + ""%s" was already declared with a different return type.", $3); + hlsl_note(ctx, &$$.decl->loc, VKD3D_SHADER_LOG_ERROR, ""%s" was previously declared here.", $3); + } + + vkd3d_free($3); + + /* We implement function invocation by copying to input + * parameters, emitting a HLSL_IR_CALL instruction, then copying + * from output parameters. As a result, we need to use the same + * parameter variables for every invocation of this function, + * which means we use the parameters created by the first + * declaration. If we're not the first declaration, the + * parameter variables that just got created will end up being + * mostly ignored—in particular, they won't be used in actual + * IR. + * + * There is a hitch: if this is the actual definition, the + * function body will look up parameter variables by name. We + * must return the original parameters, and not the ones we just + * created, but we're in the wrong scope, and the parameters + * might not even have the same names. + * + * Therefore we need to shuffle the parameters we just created + * into a dummy scope where they'll never be looked up, and + * rename the original parameters so they have the expected + * names. We actually do this for every prototype: we don't know + * whether this is the function definition yet, but it doesn't + * really matter. The variables can only be used in the + * actual definition, and don't do anything in a declaration. + * + * This is complex, and it seems tempting to avoid this logic by + * putting arguments into the HLSL_IR_CALL instruction, letting + * the canonical variables be the ones attached to the function + * definition, and resolving the copies when inlining. The + * problem with this is output parameters. We would have to use + * a lot of parsing logic on already lowered IR, which is + * brittle and ugly. + */ + + assert($5.count == params->count); + for (i = 0; i < params->count; ++i) + { + struct hlsl_ir_var *orig_param = params->vars[i]; + struct hlsl_ir_var *new_param = $5.vars[i]; + char *new_name; + + list_remove(&orig_param->scope_entry); + list_add_tail(&ctx->cur_scope->vars, &orig_param->scope_entry); + + list_remove(&new_param->scope_entry); + list_add_tail(&ctx->dummy_scope->vars, &new_param->scope_entry); + + if (!(new_name = hlsl_strdup(ctx, new_param->name))) + YYABORT; + vkd3d_free((void *)orig_param->name); + orig_param->name = new_name; + } + + $$.first = false; + $$.parameters = $5; + $$.return_semantic = $7.semantic; + } + else + { + if (!($$.decl = hlsl_new_func_decl(ctx, type, &$5, &$7.semantic, &@3))) + YYABORT; + $$.name = $3; + ctx->cur_function = $$.decl; + + $$.first = true; + } }
func_prototype: func_prototype_no_attrs | attribute_list func_prototype_no_attrs { - $2.decl->attr_count = $1.count; - $2.decl->attrs = $1.attrs; + if ($2.first) + { + $2.decl->attr_count = $1.count; + $2.decl->attrs = $1.attrs; + } + else + { + free($1.attrs); + } $$ = $2; }
From: Zebediah Figura zfigura@codeweavers.com
--- libs/vkd3d-shader/hlsl.y | 23 +++++++---------------- tests/hlsl-function.shader_test | 2 +- 2 files changed, 8 insertions(+), 17 deletions(-)
diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 8207daba..d5e2b2a9 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -77,7 +77,6 @@ struct parse_variable_def
struct parse_function { - char *name; struct hlsl_ir_function_decl *decl; struct hlsl_func_parameters parameters; struct hlsl_semantic return_semantic; @@ -3690,7 +3689,6 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl %type <fields> field %type <fields> fields_list
-%type <function> func_declaration %type <function> func_prototype %type <function> func_prototype_no_attrs
@@ -3733,17 +3731,6 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct hl hlsl_prog: %empty | hlsl_prog func_declaration - { - if ($2.first) - { - hlsl_add_function(ctx, $2.name, $2.decl); - } - else - { - vkd3d_free($2.parameters.vars); - hlsl_cleanup_semantic(&$2.return_semantic); - } - } | hlsl_prog buffer_declaration buffer_body | hlsl_prog declaration_statement { @@ -4035,11 +4022,14 @@ func_declaration: } hlsl_pop_scope(ctx);
- $$ = $1; + if (!$1.first) + { + vkd3d_free($1.parameters.vars); + hlsl_cleanup_semantic(&$1.return_semantic); + } } | func_prototype ';' { - $$ = $1; hlsl_pop_scope(ctx); }
@@ -4146,9 +4136,10 @@ func_prototype_no_attrs: { if (!($$.decl = hlsl_new_func_decl(ctx, type, &$5, &$7.semantic, &@3))) YYABORT; - $$.name = $3; ctx->cur_function = $$.decl;
+ hlsl_add_function(ctx, $3, $$.decl); + $$.first = true; } } diff --git a/tests/hlsl-function.shader_test b/tests/hlsl-function.shader_test index fa35963d..aef338d8 100644 --- a/tests/hlsl-function.shader_test +++ b/tests/hlsl-function.shader_test @@ -246,7 +246,7 @@ float4 main() : sv_target return 0; }
-[pixel shader notimpl todo] +[pixel shader notimpl]
% Even trivially finite recursion is forbidden.
Unless I am mistaken, if there is a declaration after a definition, the parameter variables will end up with the name in the last definition, e.g. in:
```hlsl float foo(float aa);
float foo(float bb) { return bb + 1; }
float foo(float cc);
float4 main() : sv_target { return foo(5); } ``` the parameter will end up with `"cc"` as its name. I think this could be a problem for error logging.
On Thu Feb 2 03:55:32 2023 +0000, Francisco Casas wrote:
Unless I am mistaken, if there is a declaration after a definition, the parameter variables will end up with the name in the last definition, e.g. in:
float foo(float aa); float foo(float bb) { return bb + 1; } float foo(float cc); float4 main() : sv_target { return foo(5); }the parameter will end up with `"cc"` as its name. I think this could be a problem for error logging.
I haven't tested, but I think it may actually be fine, or even preferred? Because the vast majority of errors are done while parsing, references to the parameters from inside the function should print "bb" here, and references to parameters from outside the function (like, say, for a type mismatch) would use the last definition or declaration (so in this case "cc"), which seems like a reasonable or even desirable behaviour.
On Thu Feb 2 03:55:31 2023 +0000, Zebediah Figura wrote:
I haven't tested, but I think it may actually be fine, or even preferred? Because the vast majority of errors are done while parsing, references to the parameters from inside the function should print "bb" here, and references to parameters from outside the function (like, say, for a type mismatch) would use the last definition or declaration (so in this case "cc"), which seems like a reasonable or even desirable behaviour.
I see. Okay then, I don't have additional remarks.
This merge request was approved by Francisco Casas.
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/hlsl.c:
return strcmp(name, type->name);}
+static struct hlsl_scope *hlsl_new_scope(struct hlsl_ctx *ctx, struct hlsl_scope *upper) +{
- struct hlsl_scope *scope;
- if (!(scope = hlsl_alloc(ctx, sizeof(*scope))))
return NULL;- TRACE("Pushing a new scope.\n");
This is already done by the caller.
Is it intentional that a couple of your commits has very long lines in the message? My understanding is that commit messages are commonly truncated at a somewhat reasonable line length.
-
Francisco Casas (＠fcasas) -
Giovanni Mascellani (＠giomasce)
-
Zebediah Figura
-
Zebediah Figura (＠zfigura)