[PATCH v2 0/5] MR310: hlsl: Define some intrinsics in HLSL.
-- v2: vkd3d-shader/hlsl: Define lit() in HLSL. vkd3d-shader/hlsl: Define smoothstep() in HLSL. vkd3d-shader/hlsl: Store the internal name counter in struct hlsl_ctx. vkd3d-shader/hlsl: Separate an add_user_call() helper. vkd3d-shader/hlsl: Introduce an hlsl_sprintf_alloc() helper. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/310
POSIX allows to use the `%m$` syntax and avoid repeat many times the same argument, but unfortunately the standard `printf()` doesn't...
Sadly, it seems that MinGW in ANSI mode doesn't implement this feature, otherwise I'd be glad to depend on it anyway. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/310#note_43776
On Tue Aug 29 09:03:30 2023 +0000, Giovanni Mascellani wrote:
I can't say I am a fan of hijacking the context in this way, but overall I can live with it. I don't love it, and it is a bit fragile. But we really do need this to be the same hlsl_ctx, and given that we only have to save a couple of members I feel like the tradeoff is *probably* worth it.
But if you look at this patch and think the tradeoffs aren't worth it, I won't find it hard to agree. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/310#note_43775
On Tue Aug 29 09:03:29 2023 +0000, Giovanni Mascellani wrote:
I think I would also be in favor of generating a new name each time an internal function is parsed. Yes, that's nicer. My original plan was to avoid compiling the same function multiple times, but I think a nicer way to do that is to cache them in dedicated hlsl_ctx entries.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/310#note_43778
From: Zebediah Figura <zfigura(a)codeweavers.com> --- libs/vkd3d-shader/d3dbc.c | 11 +++-------- libs/vkd3d-shader/hlsl.c | 21 +++++++++++++++++++++ libs/vkd3d-shader/hlsl.h | 2 ++ libs/vkd3d-shader/hlsl_codegen.c | 25 ++++++++++--------------- 4 files changed, 36 insertions(+), 23 deletions(-) diff --git a/libs/vkd3d-shader/d3dbc.c b/libs/vkd3d-shader/d3dbc.c index 99a5bd7a..2b02d51f 100644 --- a/libs/vkd3d-shader/d3dbc.c +++ b/libs/vkd3d-shader/d3dbc.c @@ -1638,17 +1638,12 @@ static void write_sm1_uniforms(struct hlsl_ctx *ctx, struct vkd3d_bytecode_buffe if (var->is_param && var->is_uniform) { - struct vkd3d_string_buffer *name; + char *new_name; - if (!(name = hlsl_get_string_buffer(ctx))) - { - buffer->status = VKD3D_ERROR_OUT_OF_MEMORY; + if (!(new_name = hlsl_sprintf_alloc(ctx, "$%s", var->name))) return; - } - vkd3d_string_buffer_printf(name, "$%s", var->name); vkd3d_free((char *)var->name); - var->name = hlsl_strdup(ctx, name->buffer); - hlsl_release_string_buffer(ctx, name); + var->name = new_name; } } } diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c index 8b706e1e..a6747cd1 100644 --- a/libs/vkd3d-shader/hlsl.c +++ b/libs/vkd3d-shader/hlsl.c @@ -72,6 +72,27 @@ void hlsl_fixme(struct hlsl_ctx *ctx, const struct vkd3d_shader_location *loc, c ctx->result = VKD3D_ERROR_NOT_IMPLEMENTED; } +char *hlsl_sprintf_alloc(struct hlsl_ctx *ctx, const char *fmt, ...) +{ + struct vkd3d_string_buffer *string; + va_list args; + char *ret; + + if (!(string = hlsl_get_string_buffer(ctx))) + return NULL; + va_start(args, fmt); + if (vkd3d_string_buffer_vprintf(string, fmt, args) < 0) + { + va_end(args); + hlsl_release_string_buffer(ctx, string); + return NULL; + } + va_end(args); + ret = hlsl_strdup(ctx, string->buffer); + hlsl_release_string_buffer(ctx, string); + return ret; +} + bool hlsl_add_var(struct hlsl_ctx *ctx, struct hlsl_ir_var *decl, bool local_var) { struct hlsl_scope *scope = ctx->cur_scope; diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index 75a1e6e4..7ff90723 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -1068,6 +1068,8 @@ static inline unsigned int hlsl_sampler_dim_count(enum hlsl_sampler_dim dim) } } +char *hlsl_sprintf_alloc(struct hlsl_ctx *ctx, const char *fmt, ...) VKD3D_PRINTF_FUNC(2, 3); + const char *debug_hlsl_expr_op(enum hlsl_ir_expr_op op); const char *debug_hlsl_type(struct hlsl_ctx *ctx, const struct hlsl_type *type); const char *debug_hlsl_writemask(unsigned int writemask); diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index bfa605f4..fbf1733f 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -163,10 +163,10 @@ static bool replace_deref_path_with_offset(struct hlsl_ctx *ctx, struct hlsl_der * work. */ static void prepend_uniform_copy(struct hlsl_ctx *ctx, struct hlsl_block *block, struct hlsl_ir_var *temp) { - struct vkd3d_string_buffer *name; struct hlsl_ir_var *uniform; struct hlsl_ir_node *store; struct hlsl_ir_load *load; + char *new_name; /* Use the synthetic name for the temp, rather than the uniform, so that we * can write the uniform name into the shader reflection data. */ @@ -180,11 +180,9 @@ static void prepend_uniform_copy(struct hlsl_ctx *ctx, struct hlsl_block *block, uniform->is_param = temp->is_param; uniform->buffer = temp->buffer; - if (!(name = hlsl_get_string_buffer(ctx))) + if (!(new_name = hlsl_sprintf_alloc(ctx, "<temp-%s>", temp->name))) return; - vkd3d_string_buffer_printf(name, "<temp-%s>", temp->name); - temp->name = hlsl_strdup(ctx, name->buffer); - hlsl_release_string_buffer(ctx, name); + temp->name = new_name; if (!(load = hlsl_new_var_load(ctx, uniform, &temp->loc))) return; @@ -235,16 +233,15 @@ static struct hlsl_ir_var *add_semantic_var(struct hlsl_ctx *ctx, struct hlsl_ir uint32_t index, bool output, const struct vkd3d_shader_location *loc) { struct hlsl_semantic new_semantic; - struct vkd3d_string_buffer *name; struct hlsl_ir_var *ext_var; + char *new_name; - if (!(name = hlsl_get_string_buffer(ctx))) + if (!(new_name = hlsl_sprintf_alloc(ctx, "<%s-%s%u>", output ? "output" : "input", semantic->name, index))) return NULL; - vkd3d_string_buffer_printf(name, "<%s-%s%u>", output ? "output" : "input", semantic->name, index); LIST_FOR_EACH_ENTRY(ext_var, &ctx->extern_vars, struct hlsl_ir_var, extern_entry) { - if (!ascii_strcasecmp(ext_var->name, name->buffer)) + if (!ascii_strcasecmp(ext_var->name, new_name)) { if (output) { @@ -271,25 +268,23 @@ static struct hlsl_ir_var *add_semantic_var(struct hlsl_ctx *ctx, struct hlsl_ir } } - hlsl_release_string_buffer(ctx, name); + vkd3d_free(new_name); return ext_var; } } if (!(new_semantic.name = hlsl_strdup(ctx, semantic->name))) { - hlsl_release_string_buffer(ctx, name); + vkd3d_free(new_name); return NULL; } new_semantic.index = index; - if (!(ext_var = hlsl_new_var(ctx, hlsl_strdup(ctx, name->buffer), type, loc, &new_semantic, - modifiers, NULL))) + if (!(ext_var = hlsl_new_var(ctx, new_name, type, loc, &new_semantic, modifiers, NULL))) { - hlsl_release_string_buffer(ctx, name); + vkd3d_free(new_name); hlsl_cleanup_semantic(&new_semantic); return NULL; } - hlsl_release_string_buffer(ctx, name); if (output) ext_var->is_output_semantic = 1; else -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/310
Is there any particular reason to add this `__attribute__((format))` in 3/4 instead of 1/4?
No, that was a mistake, thanks for catching it. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/310#note_43777
On Mon Aug 28 23:51:51 2023 +0000, Francisco Casas wrote:
Should this account for the possibility of the hlsl_sprintf_alloc() calls returning NULL? Done in v2.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/310#note_43780
From: Zebediah Figura <zfigura(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.y | 165 +++++++++++++++++++++------------------ 1 file changed, 87 insertions(+), 78 deletions(-) diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 29e0ff0c..4e3a10c8 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -2330,6 +2330,92 @@ static struct hlsl_ir_function_decl *find_function_call(struct hlsl_ctx *ctx, return args.decl; } +static struct hlsl_ir_node *hlsl_new_void_expr(struct hlsl_ctx *ctx, const struct vkd3d_shader_location *loc) +{ + struct hlsl_ir_node *operands[HLSL_MAX_OPERANDS] = {0}; + + return hlsl_new_expr(ctx, HLSL_OP0_VOID, operands, ctx->builtin_types.Void, loc); +} + +static bool add_user_call(struct hlsl_ctx *ctx, struct hlsl_ir_function_decl *func, + const struct parse_initializer *args, const struct vkd3d_shader_location *loc) +{ + struct hlsl_ir_node *call; + unsigned int i; + + assert(args->args_count == func->parameters.count); + + for (i = 0; i < func->parameters.count; ++i) + { + struct hlsl_ir_var *param = func->parameters.vars[i]; + struct hlsl_ir_node *arg = args->args[i]; + + if (!hlsl_types_are_equal(arg->data_type, param->data_type)) + { + struct hlsl_ir_node *cast; + + if (!(cast = add_cast(ctx, args->instrs, arg, param->data_type, &arg->loc))) + return false; + args->args[i] = cast; + arg = cast; + } + + if (param->storage_modifiers & HLSL_STORAGE_IN) + { + struct hlsl_ir_node *store; + + if (!(store = hlsl_new_simple_store(ctx, param, arg))) + return false; + hlsl_block_add_instr(args->instrs, store); + } + } + + if (!(call = hlsl_new_call(ctx, func, loc))) + return false; + hlsl_block_add_instr(args->instrs, call); + + for (i = 0; i < func->parameters.count; ++i) + { + struct hlsl_ir_var *param = func->parameters.vars[i]; + struct hlsl_ir_node *arg = args->args[i]; + + if (param->storage_modifiers & HLSL_STORAGE_OUT) + { + struct hlsl_ir_load *load; + + if (arg->data_type->modifiers & HLSL_MODIFIER_CONST) + hlsl_error(ctx, &arg->loc, VKD3D_SHADER_ERROR_HLSL_MODIFIES_CONST, + "Output argument to \"%s\" is const.", func->func->name); + + if (!(load = hlsl_new_var_load(ctx, param, &arg->loc))) + return false; + hlsl_block_add_instr(args->instrs, &load->node); + + if (!add_assignment(ctx, args->instrs, arg, ASSIGN_OP_ASSIGN, &load->node)) + return false; + } + } + + if (func->return_var) + { + struct hlsl_ir_load *load; + + if (!(load = hlsl_new_var_load(ctx, func->return_var, loc))) + return false; + hlsl_block_add_instr(args->instrs, &load->node); + } + else + { + struct hlsl_ir_node *expr; + + if (!(expr = hlsl_new_void_expr(ctx, loc))) + return false; + hlsl_block_add_instr(args->instrs, expr); + } + + return true; +} + static struct hlsl_ir_node *intrinsic_float_convert_arg(struct hlsl_ctx *ctx, const struct parse_initializer *params, struct hlsl_ir_node *arg, const struct vkd3d_shader_location *loc) { @@ -3659,13 +3745,6 @@ static int intrinsic_function_name_compare(const void *a, const void *b) return strcmp(a, func->name); } -static struct hlsl_ir_node *hlsl_new_void_expr(struct hlsl_ctx *ctx, const struct vkd3d_shader_location *loc) -{ - struct hlsl_ir_node *operands[HLSL_MAX_OPERANDS] = {0}; - - return hlsl_new_expr(ctx, HLSL_OP0_VOID, operands, ctx->builtin_types.Void, loc); -} - static struct hlsl_block *add_call(struct hlsl_ctx *ctx, const char *name, struct parse_initializer *args, const struct vkd3d_shader_location *loc) { @@ -3674,78 +3753,8 @@ static struct hlsl_block *add_call(struct hlsl_ctx *ctx, const char *name, if ((decl = find_function_call(ctx, name, args, loc))) { - struct hlsl_ir_node *call; - unsigned int i; - - assert(args->args_count == decl->parameters.count); - - for (i = 0; i < decl->parameters.count; ++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)) - { - struct hlsl_ir_node *cast; - - if (!(cast = add_cast(ctx, args->instrs, arg, param->data_type, &arg->loc))) - goto fail; - args->args[i] = cast; - arg = cast; - } - - if (param->storage_modifiers & HLSL_STORAGE_IN) - { - struct hlsl_ir_node *store; - - if (!(store = hlsl_new_simple_store(ctx, param, arg))) - goto fail; - hlsl_block_add_instr(args->instrs, store); - } - } - - if (!(call = hlsl_new_call(ctx, decl, loc))) + if (!add_user_call(ctx, decl, args, loc)) goto fail; - hlsl_block_add_instr(args->instrs, call); - - for (i = 0; i < decl->parameters.count; ++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) - { - struct hlsl_ir_load *load; - - if (arg->data_type->modifiers & HLSL_MODIFIER_CONST) - hlsl_error(ctx, &arg->loc, VKD3D_SHADER_ERROR_HLSL_MODIFIES_CONST, - "Output argument to \"%s\" is const.", decl->func->name); - - if (!(load = hlsl_new_var_load(ctx, param, &arg->loc))) - goto fail; - hlsl_block_add_instr(args->instrs, &load->node); - - if (!add_assignment(ctx, args->instrs, arg, ASSIGN_OP_ASSIGN, &load->node)) - goto fail; - } - } - - if (decl->return_var) - { - struct hlsl_ir_load *load; - - if (!(load = hlsl_new_var_load(ctx, decl->return_var, loc))) - goto fail; - hlsl_block_add_instr(args->instrs, &load->node); - } - else - { - struct hlsl_ir_node *expr; - - if (!(expr = hlsl_new_void_expr(ctx, loc))) - goto fail; - hlsl_block_add_instr(args->instrs, expr); - } } else if ((intrinsic = bsearch(name, intrinsic_functions, ARRAY_SIZE(intrinsic_functions), sizeof(*intrinsic_functions), intrinsic_function_name_compare))) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/310
On Mon Aug 28 23:51:50 2023 +0000, Francisco Casas wrote:
I think we should ```c vkd3d_free(new_name); ``` here. Good catch, done in v2.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/310#note_43779
From: Zebediah Figura <zfigura(a)codeweavers.com> This is minutely more efficient than using a global variable and atomic instructions. --- libs/vkd3d-shader/hlsl.c | 3 +-- libs/vkd3d-shader/hlsl.h | 3 +++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c index a6747cd1..be5b4052 100644 --- a/libs/vkd3d-shader/hlsl.c +++ b/libs/vkd3d-shader/hlsl.c @@ -1060,11 +1060,10 @@ struct hlsl_ir_var *hlsl_new_synthetic_var(struct hlsl_ctx *ctx, const char *tem { struct vkd3d_string_buffer *string; struct hlsl_ir_var *var; - static LONG counter; if (!(string = hlsl_get_string_buffer(ctx))) return NULL; - vkd3d_string_buffer_printf(string, "<%s-%u>", template, InterlockedIncrement(&counter)); + vkd3d_string_buffer_printf(string, "<%s-%u>", template, ctx->internal_name_counter++); var = hlsl_new_synthetic_var_named(ctx, string->buffer, type, loc, true); hlsl_release_string_buffer(ctx, string); return var; diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index 7ff90723..567e2908 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -797,6 +797,9 @@ struct hlsl_ctx /* Pointer to the current function; changes as the parser reads the code. */ const struct hlsl_ir_function_decl *cur_function; + /* Counter for generating unique internal variable names. */ + unsigned int internal_name_counter; + /* Default matrix majority for matrix types. Can be set by a pragma within the HLSL source. */ unsigned int matrix_majority; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/310
Is the change from `return` to `continue` intentional? If there is a memory allocation error I guess we should unwind the stack as soon as possible. Maybe even allowing `write_sm1_uniforms()` and similar functions to return an error.
Probably, but I don't think I have a good reason for it. Changed back in v2. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/310#note_43781
From: Zebediah Figura <zfigura(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.c | 51 +++++++++++++++++++++++++++++++++ libs/vkd3d-shader/hlsl.h | 8 ++++++ libs/vkd3d-shader/hlsl.y | 61 +++++++++++----------------------------- 3 files changed, 75 insertions(+), 45 deletions(-) diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c index be5b4052..b8cf6813 100644 --- a/libs/vkd3d-shader/hlsl.c +++ b/libs/vkd3d-shader/hlsl.c @@ -2988,6 +2988,16 @@ void hlsl_add_function(struct hlsl_ctx *ctx, char *name, struct hlsl_ir_function struct hlsl_ir_function *func; struct rb_entry *func_entry; + if (ctx->internal_func_name) + { + char *internal_name; + + if (!(internal_name = hlsl_strdup(ctx, ctx->internal_func_name))) + return; + vkd3d_free(name); + name = internal_name; + } + func_entry = rb_get(&ctx->functions, name); if (func_entry) { @@ -3519,3 +3529,44 @@ int hlsl_compile_shader(const struct vkd3d_shader_code *hlsl, const struct vkd3d hlsl_ctx_cleanup(&ctx); return ret; } + +struct hlsl_ir_function_decl *hlsl_compile_internal_function(struct hlsl_ctx *ctx, const char *name, const char *hlsl) +{ + const struct hlsl_ir_function_decl *saved_cur_function = ctx->cur_function; + struct vkd3d_shader_code code = {.code = hlsl, .size = strlen(hlsl)}; + const char *saved_internal_func_name = ctx->internal_func_name; + struct vkd3d_string_buffer *internal_name; + struct hlsl_ir_function_decl *func; + void *saved_scanner = ctx->scanner; + int ret; + + TRACE("name %s, hlsl %s.\n", debugstr_a(name), debugstr_a(hlsl)); + + /* The actual name of the function is mangled with a unique prefix, both to + * allow defining multiple variants of a function with the same name, and to + * avoid polluting the user name space. */ + + if (!(internal_name = hlsl_get_string_buffer(ctx))) + return NULL; + vkd3d_string_buffer_printf(internal_name, "<%s-%u>", name, ctx->internal_name_counter++); + + /* Save and restore everything that matters. + * Note that saving the scope stack is hard, and shouldn't be necessary. */ + + ctx->scanner = NULL; + ctx->internal_func_name = internal_name->buffer; + ctx->cur_function = NULL; + ret = hlsl_lexer_compile(ctx, &code); + ctx->scanner = saved_scanner; + ctx->internal_func_name = saved_internal_func_name; + ctx->cur_function = saved_cur_function; + if (ret) + { + ERR("Failed to compile intrinsic, error %u.\n", ret); + hlsl_release_string_buffer(ctx, internal_name); + return NULL; + } + func = hlsl_get_func_decl(ctx, internal_name->buffer); + hlsl_release_string_buffer(ctx, internal_name); + return func; +} diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index 567e2908..26b0b89e 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -836,6 +836,12 @@ struct hlsl_ctx * compute shader profiles. It is set using the numthreads() attribute in the entry point. */ uint32_t thread_count[3]; + /* In some cases we generate opcodes by parsing an HLSL function and then + * invoking it. If not NULL, this field is the name of the function that we + * are currently parsing, "mangled" with an internal prefix to avoid + * polluting the user namespace. */ + const char *internal_func_name; + /* Whether the parser is inside a state block (effects' metadata) inside a variable declaration. */ uint32_t in_state_block : 1; /* Whether the numthreads() attribute has been provided in the entry-point function. */ @@ -1262,6 +1268,8 @@ bool hlsl_sm4_register_from_semantic(struct hlsl_ctx *ctx, const struct hlsl_sem bool output, enum vkd3d_shader_register_type *type, enum vkd3d_sm4_swizzle_type *swizzle_type, bool *has_idx); int hlsl_sm4_write(struct hlsl_ctx *ctx, struct hlsl_ir_function_decl *entry_func, struct vkd3d_shader_code *out); +struct hlsl_ir_function_decl *hlsl_compile_internal_function(struct hlsl_ctx *ctx, const char *name, const char *hlsl); + int hlsl_lexer_compile(struct hlsl_ctx *ctx, const struct vkd3d_shader_code *hlsl); #endif diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 4e3a10c8..d1854fde 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -3422,58 +3422,29 @@ static bool intrinsic_sin(struct hlsl_ctx *ctx, static bool intrinsic_smoothstep(struct hlsl_ctx *ctx, const struct parse_initializer *params, const struct vkd3d_shader_location *loc) { - struct hlsl_ir_node *min_arg, *max_arg, *x_arg, *p, *p_num, *p_denom, *res, *one, *minus_two, *three; - - if (!elementwise_intrinsic_float_convert_args(ctx, params, loc)) - return false; - - min_arg = params->args[0]; - max_arg = params->args[1]; - x_arg = params->args[2]; - - if (!(min_arg = add_unary_arithmetic_expr(ctx, params->instrs, HLSL_OP1_NEG, min_arg, loc))) - return false; - - if (!(p_num = add_binary_arithmetic_expr(ctx, params->instrs, HLSL_OP2_ADD, x_arg, min_arg, loc))) - return false; - - if (!(p_denom = add_binary_arithmetic_expr(ctx, params->instrs, HLSL_OP2_ADD, max_arg, min_arg, loc))) - return false; - - if (!(one = hlsl_new_float_constant(ctx, 1.0, loc))) - return false; - hlsl_block_add_instr(params->instrs, one); - - if (!(p_denom = add_binary_arithmetic_expr(ctx, params->instrs, HLSL_OP2_DIV, one, p_denom, loc))) - return false; - - if (!(p = add_binary_arithmetic_expr(ctx, params->instrs, HLSL_OP2_MUL, p_num, p_denom, loc))) - return false; - - if (!(p = add_unary_arithmetic_expr(ctx, params->instrs, HLSL_OP1_SAT, p, loc))) - return false; - - if (!(minus_two = hlsl_new_float_constant(ctx, -2.0, loc))) - return false; - hlsl_block_add_instr(params->instrs, minus_two); - - if (!(three = hlsl_new_float_constant(ctx, 3.0, loc))) - return false; - hlsl_block_add_instr(params->instrs, three); + struct hlsl_ir_function_decl *func; + struct hlsl_type *type; + char *body; - if (!(res = add_binary_arithmetic_expr(ctx, params->instrs, HLSL_OP2_MUL, minus_two, p, loc))) - return false; + static const char template[] = + "%s smoothstep(%s low, %s high, %s x)\n" + "{\n" + " %s p = saturate((x - low) / (high - low));\n" + " return (p * p) * (3 - 2 * p);\n" + "}"; - if (!(res = add_binary_arithmetic_expr(ctx, params->instrs, HLSL_OP2_ADD, three, res, loc))) + if (!(type = elementwise_intrinsic_get_common_type(ctx, params, loc))) return false; + type = hlsl_get_numeric_type(ctx, type->class, HLSL_TYPE_FLOAT, type->dimx, type->dimy); - if (!(p = add_binary_arithmetic_expr(ctx, params->instrs, HLSL_OP2_MUL, p, p, loc))) + if (!(body = hlsl_sprintf_alloc(ctx, template, type->name, type->name, type->name, type->name, type->name))) return false; - - if (!(res = add_binary_arithmetic_expr(ctx, params->instrs, HLSL_OP2_MUL, p, res, loc))) + func = hlsl_compile_internal_function(ctx, "smoothstep", body); + vkd3d_free(body); + if (!func) return false; - return true; + return add_user_call(ctx, func, params, loc); } static bool intrinsic_sqrt(struct hlsl_ctx *ctx, -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/310
From: Zebediah Figura <zfigura(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.y | 83 ++++++-------------------------------- tests/hlsl/lit.shader_test | 11 +++++ 2 files changed, 24 insertions(+), 70 deletions(-) diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index d1854fde..2775c37e 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -3034,14 +3034,17 @@ static struct hlsl_ir_node * add_pow_expr(struct hlsl_ctx *ctx, static bool intrinsic_lit(struct hlsl_ctx *ctx, const struct parse_initializer *params, const struct vkd3d_shader_location *loc) { - struct hlsl_ir_node *n_l_neg, *n_h_neg, *specular_or, *specular_pow, *load; - struct hlsl_ir_node *n_l, *n_h, *m, *diffuse, *zero, *store, *init; - struct hlsl_constant_value init_value; - struct hlsl_ir_load *var_load; - struct hlsl_deref var_deref; - struct hlsl_type *ret_type; - struct hlsl_ir_var *var; - struct hlsl_block block; + struct hlsl_ir_function_decl *func; + + static const char body[] = + "float4 lit(float n_l, float n_h, float m)\n" + "{\n" + " float4 ret;\n" + " ret.xw = 1.0;\n" + " ret.y = max(n_l, 0);\n" + " ret.z = (n_l < 0 || n_h < 0) ? 0 : pow(n_h, m);\n" + " return ret;\n" + "}"; if (params->args[0]->data_type->class != HLSL_CLASS_SCALAR || params->args[1]->data_type->class != HLSL_CLASS_SCALAR @@ -3051,70 +3054,10 @@ static bool intrinsic_lit(struct hlsl_ctx *ctx, return false; } - if (!(n_l = intrinsic_float_convert_arg(ctx, params, params->args[0], loc))) - return false; - - if (!(n_h = intrinsic_float_convert_arg(ctx, params, params->args[1], loc))) + if (!(func = hlsl_compile_internal_function(ctx, "lit", body))) return false; - if (!(m = intrinsic_float_convert_arg(ctx, params, params->args[2], loc))) - return false; - - ret_type = hlsl_get_vector_type(ctx, HLSL_TYPE_FLOAT, 4); - - if (!(var = hlsl_new_synthetic_var(ctx, "lit", ret_type, loc))) - return false; - hlsl_init_simple_deref_from_var(&var_deref, var); - - init_value.u[0].f = 1.0f; - init_value.u[1].f = 0.0f; - init_value.u[2].f = 0.0f; - init_value.u[3].f = 1.0f; - if (!(init = hlsl_new_constant(ctx, ret_type, &init_value, loc))) - return false; - hlsl_block_add_instr(params->instrs, init); - - if (!(store = hlsl_new_simple_store(ctx, var, init))) - return false; - hlsl_block_add_instr(params->instrs, store); - - if (!(zero = hlsl_new_float_constant(ctx, 0.0f, loc))) - return false; - hlsl_block_add_instr(params->instrs, zero); - - /* Diffuse component. */ - if (!(diffuse = add_binary_arithmetic_expr(ctx, params->instrs, HLSL_OP2_MAX, n_l, zero, loc))) - return false; - - if (!hlsl_new_store_component(ctx, &block, &var_deref, 1, diffuse)) - return false; - hlsl_block_add_block(params->instrs, &block); - - /* Specular component. */ - if (!(n_h_neg = add_binary_comparison_expr(ctx, params->instrs, HLSL_OP2_LESS, n_h, zero, loc))) - return false; - - if (!(n_l_neg = add_binary_comparison_expr(ctx, params->instrs, HLSL_OP2_LESS, n_l, zero, loc))) - return false; - - if (!(specular_or = add_binary_logical_expr(ctx, params->instrs, HLSL_OP2_LOGIC_OR, n_l_neg, n_h_neg, loc))) - return false; - - if (!(specular_pow = add_pow_expr(ctx, params->instrs, n_h, m, loc))) - return false; - - if (!(load = hlsl_add_conditional(ctx, params->instrs, specular_or, zero, specular_pow))) - return false; - - if (!hlsl_new_store_component(ctx, &block, &var_deref, 2, load)) - return false; - hlsl_block_add_block(params->instrs, &block); - - if (!(var_load = hlsl_new_var_load(ctx, var, loc))) - return false; - hlsl_block_add_instr(params->instrs, &var_load->node); - - return true; + return add_user_call(ctx, func, params, loc); } static bool intrinsic_log(struct hlsl_ctx *ctx, diff --git a/tests/hlsl/lit.shader_test b/tests/hlsl/lit.shader_test index 729acef5..cbfffb4e 100644 --- a/tests/hlsl/lit.shader_test +++ b/tests/hlsl/lit.shader_test @@ -19,6 +19,17 @@ uniform 0 float4 1.2 2.0 3.0 0.0 draw quad probe all rgba (1.0, 1.2, 8.0, 1.0) +[pixel shader] +float4 main(uniform float4 u) : sv_target +{ + return lit(u.x, u.y, u.z) + lit(u.x, u.y, u.z); +} + +[test] +uniform 0 float4 1.2 2.0 3.0 0.0 +draw quad +probe all rgba (2.0, 2.4, 16.0, 2.0) + [pixel shader fail] float4 main(uniform float4 u) : sv_target { -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/310
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/hlsl.c:
{ struct vkd3d_string_buffer *string; struct hlsl_ir_var *var; - static LONG counter;
if (!(string = hlsl_get_string_buffer(ctx))) return NULL; - vkd3d_string_buffer_printf(string, "<%s-%u>", template, InterlockedIncrement(&counter)); + vkd3d_string_buffer_printf(string, "<%s-%u>", template, ctx->internal_name_counter++); This made me notice that we don't verify the return value for `vkd3d_string_buffer_printf()`. And apparently it's endemic in vkd3d-shader, so I can't really blame it on you, but maybe it makes sense to start rectifying it? The same happens in 4/5.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/310#note_43853
This merge request was approved by Giovanni Mascellani. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/310
This made me notice that we don't verify the return value for `vkd3d_string_buffer_printf()`. And apparently it's endemic in vkd3d-shader, so I can't really blame it on you, but maybe it makes sense to start rectifying it?
Mostly because it's not terribly practical to check in anything that outputs more than one or two lines. I think we should adopt the vkd3d_bytecode_buffer scheme and track the status on the vkd3d_string_buffer instead of attempting to propagate errors up from vkd3d_string_buffer_resize().
POSIX allows to use the `%m$` syntax and avoid repeat many times the same argument, but unfortunately the standard `printf()` doesn't...
Sadly, it seems that MinGW in ANSI mode doesn't implement this feature, otherwise I'd be glad to depend on it anyway.
Could we perhaps use _vsprintf_p() on the only non-POSIX platform we care about? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/310#note_43892
This merge request was approved by Henri Verbeet. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/310
This merge request was approved by Francisco Casas. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/310
participants (5)
-
Francisco Casas (＠fcasas) -
Giovanni Mascellani (＠giomasce)
-
Henri Verbeet (＠hverbeet)
-
Zebediah Figura
-
Zebediah Figura (＠zfigura)