Currently we are assuming that unknown identifiers within state blocks are scalar integers. This proposal keeps doing this.
The lower_static_constant_folding() meta-pass is introduced to fold as much as possible these state state block expressions. And I plan to reuse it for constant buffer default values.
A lower_state_block_identifier_loads() pass is included in the last commit. The idea is to start filling a table with numeric values for the unknown identifiers as we start discovering them.
From: Francisco Casas fcasas@codeweavers.com
--- libs/vkd3d-shader/hlsl.c | 32 +++++++++++++++++++++++++++ libs/vkd3d-shader/hlsl.h | 16 ++++++++++++++ libs/vkd3d-shader/hlsl.y | 47 ++++++++++++++++++++++++++++++++++------ 3 files changed, 88 insertions(+), 7 deletions(-)
diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c index 3d068ac6d..8a4b9a4ef 100644 --- a/libs/vkd3d-shader/hlsl.c +++ b/libs/vkd3d-shader/hlsl.c @@ -134,6 +134,19 @@ struct hlsl_ir_var *hlsl_get_var(struct hlsl_scope *scope, const char *name) return hlsl_get_var(scope->upper, name); }
+void hlsl_free_state_block(struct hlsl_state_block *sblock) +{ + unsigned int k; + + assert(sblock); + for (k = 0; k < sblock->count; ++k) + { + hlsl_block_cleanup(sblock->entries[k].rhs_instrs); + vkd3d_free(sblock->entries[k].rhs_instrs); + } + vkd3d_free(sblock); +} + void hlsl_free_var(struct hlsl_ir_var *decl) { unsigned int k; @@ -142,6 +155,11 @@ void hlsl_free_var(struct hlsl_ir_var *decl) hlsl_cleanup_semantic(&decl->semantic); for (k = 0; k <= HLSL_REGSET_LAST_OBJECT; ++k) vkd3d_free((void *)decl->objects_usage[k]); + if (decl->state_block) + { + hlsl_free_state_block(decl->state_block); + decl->state_block = NULL; + } vkd3d_free(decl); }
@@ -3591,6 +3609,20 @@ static void hlsl_ctx_cleanup(struct hlsl_ctx *ctx)
rb_destroy(&ctx->functions, free_function_rb, NULL);
+ /* State blocks must be free before the variables, because they contain instructions that may + * refer to them. */ + LIST_FOR_EACH_ENTRY_SAFE(scope, next_scope, &ctx->scopes, struct hlsl_scope, entry) + { + LIST_FOR_EACH_ENTRY_SAFE(var, next_var, &scope->vars, struct hlsl_ir_var, scope_entry) + { + if (var->state_block) + { + hlsl_free_state_block(var->state_block); + var->state_block = NULL; + } + } + } + LIST_FOR_EACH_ENTRY_SAFE(scope, next_scope, &ctx->scopes, struct hlsl_scope, entry) { LIST_FOR_EACH_ENTRY_SAFE(var, next_var, &scope->vars, struct hlsl_ir_var, scope_entry) diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index 43bc079db..d46675e70 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -413,6 +413,9 @@ struct hlsl_ir_var /* Scope that contains annotations for this variable. */ struct hlsl_scope *annotations;
+ /* The state_block on the variable's declaration, if it has one. */ + struct hlsl_state_block *state_block; + /* Indexes of the IR instructions where the variable is first written and last read (liveness * range). The IR instructions are numerated starting from 2, because 0 means unused, and 1 * means function entry. */ @@ -448,6 +451,18 @@ struct hlsl_ir_var uint32_t is_separated_resource : 1; };
+struct hlsl_state_block_entry +{ + char *lhs_name; + struct hlsl_block *rhs_instrs; +}; + +struct hlsl_state_block +{ + struct hlsl_state_block_entry *entries; + size_t count, capacity; +}; + /* Sized array of variables representing a function's parameters. */ struct hlsl_func_parameters { @@ -1188,6 +1203,7 @@ void hlsl_replace_node(struct hlsl_ir_node *old, struct hlsl_ir_node *new); void hlsl_free_attribute(struct hlsl_attribute *attr); void hlsl_free_instr(struct hlsl_ir_node *node); void hlsl_free_instr_list(struct list *list); +void hlsl_free_state_block(struct hlsl_state_block *sblock); void hlsl_free_type(struct hlsl_type *type); void hlsl_free_var(struct hlsl_ir_var *decl);
diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 37a372893..d88de8a6a 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -77,6 +77,8 @@ struct parse_variable_def struct hlsl_type *basic_type; uint32_t modifiers; struct vkd3d_shader_location modifiers_loc; + + struct hlsl_state_block *state_block; };
struct parse_function @@ -925,6 +927,8 @@ static void free_parse_variable_def(struct parse_variable_def *v) vkd3d_free(v->arrays.sizes); vkd3d_free(v->name); hlsl_cleanup_semantic(&v->semantic); + if (v->state_block) + hlsl_free_state_block(v->state_block); vkd3d_free(v); }
@@ -2343,6 +2347,9 @@ static struct hlsl_block *initialize_vars(struct hlsl_ctx *ctx, struct list *var free_parse_variable_def(v); continue; } + + var->state_block = v->state_block; + v->state_block = NULL; type = var->data_type;
if (v->initializer.args_count) @@ -5047,6 +5054,16 @@ static void validate_uav_type(struct hlsl_ctx *ctx, enum hlsl_sampler_dim dim, hlsl_release_string_buffer(ctx, string); }
+static bool state_block_add_entry(struct hlsl_state_block *stateb, struct hlsl_state_block_entry entry) +{ + if (!vkd3d_array_reserve((void **)&stateb->entries, &stateb->capacity, stateb->count + 1, + sizeof(*stateb->entries))) + return false; + + stateb->entries[stateb->count++] = entry; + return true; +} + }
%locations @@ -5087,6 +5104,8 @@ static void validate_uav_type(struct hlsl_ctx *ctx, enum hlsl_sampler_dim dim, struct parse_attribute_list attr_list; struct hlsl_ir_switch_case *switch_case; struct hlsl_scope *scope; + struct hlsl_state_block *state_block; + struct hlsl_state_block_entry state_block_entry; }
%token KW_BLENDSTATE @@ -5312,6 +5331,9 @@ static void validate_uav_type(struct hlsl_ctx *ctx, enum hlsl_sampler_dim dim, %type <variable_def> variable_def %type <variable_def> variable_def_typed
+%type <state_block_entry> state +%type <state_block> state_block + %%
hlsl_prog: @@ -6427,20 +6449,29 @@ variable_decl:
state: any_identifier '=' expr ';' - { - vkd3d_free($1); - destroy_block($3); - } + { + $$.lhs_name = $1; + $$.rhs_instrs = $3; + }
state_block_start: %empty - { - ctx->in_state_block = 1; - } + { + ctx->in_state_block = 1; + hlsl_push_scope(ctx); + }
state_block: %empty + { + if (!($$ = hlsl_alloc(ctx, sizeof(*$$)))) + YYABORT; + } | state_block state + { + $$ = $1; + state_block_add_entry($$, $2); + }
variable_def: variable_decl @@ -6452,7 +6483,9 @@ variable_def: | variable_decl '{' state_block_start state_block '}' { $$ = $1; + $$->state_block = $4; ctx->in_state_block = 0; + hlsl_pop_scope(ctx); }
variable_def_typed:
From: Francisco Casas fcasas@codeweavers.com
---
We may prefer to call hlsl_dump_state_block() somewhere else, like in fx.c. But for now this will do. --- libs/vkd3d-shader/hlsl.c | 18 ++++++++++++++++++ libs/vkd3d-shader/hlsl.h | 1 + libs/vkd3d-shader/hlsl.y | 4 ++++ 3 files changed, 23 insertions(+)
diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c index 8a4b9a4ef..dc07f4164 100644 --- a/libs/vkd3d-shader/hlsl.c +++ b/libs/vkd3d-shader/hlsl.c @@ -2898,6 +2898,24 @@ void hlsl_dump_function(struct hlsl_ctx *ctx, const struct hlsl_ir_function_decl vkd3d_string_buffer_cleanup(&buffer); }
+void hlsl_dump_state_block(struct hlsl_ctx *ctx, const char *var_name, const struct hlsl_state_block *stateb) +{ + struct vkd3d_string_buffer buffer; + unsigned int i; + + vkd3d_string_buffer_init(&buffer); + vkd3d_string_buffer_printf(&buffer, "Dumping state block for variable '%s' (count: %lu).\n", var_name, stateb->count); + + for (i = 0; i < stateb->count; ++i) + { + vkd3d_string_buffer_printf(&buffer, "Field '%s':\n", stateb->entries[i].lhs_name); + dump_block(ctx, &buffer, stateb->entries[i].rhs_instrs); + } + + vkd3d_string_buffer_trace(&buffer); + vkd3d_string_buffer_cleanup(&buffer); +} + void hlsl_replace_node(struct hlsl_ir_node *old, struct hlsl_ir_node *new) { struct hlsl_src *src, *next; diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index d46675e70..892ae4497 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -1184,6 +1184,7 @@ void hlsl_block_cleanup(struct hlsl_block *block); bool hlsl_clone_block(struct hlsl_ctx *ctx, struct hlsl_block *dst_block, const struct hlsl_block *src_block);
void hlsl_dump_function(struct hlsl_ctx *ctx, const struct hlsl_ir_function_decl *func); +void hlsl_dump_state_block(struct hlsl_ctx *ctx, const char *var_name, const struct hlsl_state_block *stateb);
int hlsl_emit_bytecode(struct hlsl_ctx *ctx, struct hlsl_ir_function_decl *entry_func, enum vkd3d_shader_target_type target_type, struct vkd3d_shader_code *out); diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index d88de8a6a..944f0f5f0 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -6484,6 +6484,10 @@ variable_def: { $$ = $1; $$->state_block = $4; + + if (TRACE_ON()) + hlsl_dump_state_block(ctx, $1->name, $4); + ctx->in_state_block = 0; hlsl_pop_scope(ctx); }
From: Francisco Casas fcasas@codeweavers.com
--- libs/vkd3d-shader/hlsl.h | 2 ++ libs/vkd3d-shader/hlsl.y | 1 + libs/vkd3d-shader/hlsl_codegen.c | 11 +++++++++++ 3 files changed, 14 insertions(+)
diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index 892ae4497..cab5331ba 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -1190,6 +1190,8 @@ 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); int hlsl_emit_effect_binary(struct hlsl_ctx *ctx, struct vkd3d_shader_code *out);
+void hlsl_static_constant_folding(struct hlsl_ctx *ctx, struct hlsl_block *expr); + bool hlsl_init_deref_from_index_chain(struct hlsl_ctx *ctx, struct hlsl_deref *deref, struct hlsl_ir_node *chain); bool hlsl_copy_deref(struct hlsl_ctx *ctx, struct hlsl_deref *deref, const struct hlsl_deref *other);
diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 944f0f5f0..87b1034e6 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -6451,6 +6451,7 @@ state: any_identifier '=' expr ';' { $$.lhs_name = $1; + hlsl_static_constant_folding(ctx, $3); $$.rhs_instrs = $3; }
diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index 4121fadf3..383a32f55 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -5159,3 +5159,14 @@ int hlsl_emit_bytecode(struct hlsl_ctx *ctx, struct hlsl_ir_function_decl *entry return VKD3D_ERROR_INVALID_ARGUMENT; } } + +void hlsl_static_constant_folding(struct hlsl_ctx *ctx, struct hlsl_block *expr) +{ + bool progress; + + do + { + progress = hlsl_transform_ir(ctx, hlsl_fold_constant_exprs, expr, NULL); + progress |= hlsl_copy_propagation_execute(ctx, expr); + } while (progress); +}
From: Francisco Casas fcasas@codeweavers.com
---
We would want to add values to known_identifiers[] as we identify them.
I don't know how to handle unknown identifiers. Should we set them to zero with a respective warning ? --- libs/vkd3d-shader/hlsl.c | 2 ++ libs/vkd3d-shader/hlsl.h | 3 ++ libs/vkd3d-shader/hlsl.y | 1 + libs/vkd3d-shader/hlsl_codegen.c | 47 ++++++++++++++++++++++++++++++++ 4 files changed, 53 insertions(+)
diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c index dc07f4164..66948decc 100644 --- a/libs/vkd3d-shader/hlsl.c +++ b/libs/vkd3d-shader/hlsl.c @@ -160,6 +160,8 @@ void hlsl_free_var(struct hlsl_ir_var *decl) hlsl_free_state_block(decl->state_block); decl->state_block = NULL; } + if (decl->state_block_identifier) + vkd3d_free((void *)decl->state_block_identifier); vkd3d_free(decl); }
diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index cab5331ba..5f8daca2e 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -415,6 +415,9 @@ struct hlsl_ir_var
/* The state_block on the variable's declaration, if it has one. */ struct hlsl_state_block *state_block; + /* If the variable appeared as a NEW_IDENTIFIER token in a rhs of a state block, this stores + * the name of the identifier. */ + const char *state_block_identifier;
/* Indexes of the IR instructions where the variable is first written and last read (liveness * range). The IR instructions are numerated starting from 2, because 0 means unused, and 1 diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 87b1034e6..7cc29650a 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -7113,6 +7113,7 @@ primary_expr: if (!(var = hlsl_new_synthetic_var(ctx, "state_block_expr", hlsl_get_scalar_type(ctx, HLSL_TYPE_INT), &@1))) YYABORT; + var->state_block_identifier = $1; if (!(load = hlsl_new_var_load(ctx, var, &@1))) YYABORT; if (!($$ = make_block(ctx, &load->node))) diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index 383a32f55..e0338a5e4 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -5160,10 +5160,57 @@ int hlsl_emit_bytecode(struct hlsl_ctx *ctx, struct hlsl_ir_function_decl *entry } }
+static bool lower_state_block_identifier_loads(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, void *context) +{ + const char *identifier = NULL; + struct hlsl_ir_load *load; + int i; + + static const struct + { + const char *name; + union hlsl_constant_value_component value; + } + known_identifiers[] = + { + /* We must fill this table as we start discovering the values of the identifiers. */ + {"EXAMPLE", {.u = 25}}, + }; + + if (instr->type != HLSL_IR_LOAD) + return false; + load = hlsl_ir_load(instr); + + if (!(identifier = load->src.var->state_block_identifier)) + return false; + + for (i = 0; i < ARRAY_SIZE(known_identifiers); ++i) + { + if (!strcmp(identifier, known_identifiers[i].name)) + { + struct hlsl_constant_value value = {0}; + struct hlsl_ir_node *c; + + value.u[0] = known_identifiers[i].value; + + if (!(c = hlsl_new_constant(ctx, instr->data_type, &value, &instr->loc))) + return false; + hlsl_replace_node(instr, c); + return true; + } + } + + FIXME("Unkown value of identifier '%s'\n", identifier); + return false; +} + void hlsl_static_constant_folding(struct hlsl_ctx *ctx, struct hlsl_block *expr) { bool progress;
+ if (ctx->in_state_block) + hlsl_transform_ir(ctx, lower_state_block_identifier_loads, expr, NULL); + do { progress = hlsl_transform_ir(ctx, hlsl_fold_constant_exprs, expr, NULL);
Nikolay Sivov (@nsivov) commented about libs/vkd3d-shader/hlsl.h:
uint32_t is_separated_resource : 1;};
+struct hlsl_state_block_entry +{
- char *lhs_name;
- struct hlsl_block *rhs_instrs;
+};
Full syntax for an assignment is:
field_name = value; | field_name[index] = value;
Field name is indeed just a string, meaning we don't need to validate it when compiling for regular profiles, like ps/vs. It's validated only for fx profiles, so we get do it later at writing time. For error reporting it might be beneficial to add location for lhs name.
Index here is optional when parsing, some fields are not arrays. Index value is strictly a non-negative literal integer. Having 0 for a case when index syntax is not used is fine - zero is what's getting written, at least for fx_4+.
For a field value it's more complicated. It could take a few forms:
<var_name> - I think that's used for fx_2_0 only, and var_name is for example a texture object set for Texture field. expression that resolves to a literal numeric value - could be bool, int, or float value. It's later is coerced accordingly. var_name - sets value dynamically to a variable. var_name[literal_index] - sets value to a fixed element of a variable. var_name[var_index] - same, but index is a variable too. expression - expression here could be a lot of thing, including predefined function calls.
My point is, having hlsl_block here seems useful at first, but in practice it's only useful for expressions, and even then it's not compatible with them, because "instruction set" used for expressions is not the same as for the rest of hlsl.
Were you thinking about using the same structure for passes as well? I think it could be used, but maybe will need to expand a bit to have a set/array of values for a single field. For older style passes used same "name = value;" assignment syntax, but fx_4+ is using function-looking syntax, like SetPixelShader(...), and those occasionally have multiple arguments. No problem in using "SetPixelShader" as a field name here for consistency here.
On Tue Mar 12 14:59:13 2024 +0000, Nikolay Sivov wrote:
Full syntax for an assignment is: field_name = value; | field_name[index] = value; Field name is indeed just a string, meaning we don't need to validate it when compiling for regular profiles, like ps/vs. It's validated only for fx profiles, so we get do it later at writing time. For error reporting it might be beneficial to add location for lhs name. Index here is optional when parsing, some fields are not arrays. Index value is strictly a non-negative literal integer. Having 0 for a case when index syntax is not used is fine - zero is what's getting written, at least for fx_4+. For a field value it's more complicated. It could take a few forms: <var_name> - I think that's used for fx_2_0 only, and var_name is for example a texture object set for Texture field. expression that resolves to a literal numeric value - could be bool, int, or float value. It's later is coerced accordingly. var_name - sets value dynamically to a variable. var_name[literal_index] - sets value to a fixed element of a variable. var_name[var_index] - same, but index is a variable too. expression - expression here could be a lot of thing, including predefined function calls. My point is, having hlsl_block here seems useful at first, but in practice it's only useful for expressions, and even then it's not compatible with them, because "instruction set" used for expressions is not the same as for the rest of hlsl.
Still, I think that hlsl_block covers _expression_ and all the other cases. Using node_from_block(), it can be checked if the resulting node is an HLSL_IR_LOAD for the other cases.
Even if the "instruction set" is different, it is probably a restrictive subset of the instruction set handled by the hlsl, right?
On Tue Mar 12 16:27:35 2024 +0000, Nikolay Sivov wrote:
Were you thinking about using the same structure for passes as well? I think it could be used, but maybe will need to expand a bit to have a set/array of values for a single field. For older style passes used same "name = value;" assignment syntax, but fx_4+ is using function-looking syntax, like SetPixelShader(...), and those occasionally have multiple arguments. No problem in using "SetPixelShader" as a field name here for consistency here.
Can you give me or point me to an example of these effect passes? I find it difficult to find, maybe I am not searching properly.
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.c:
return hlsl_get_var(scope->upper, name);}
+void hlsl_free_state_block(struct hlsl_state_block *sblock)
This is sometimes "stateb" and sometimes "sblock". Both seem odd to me, I'd personally use "stateblock" or "state_block" or maybe "sb", but I'd mostly prefer to pick one and stick with it.
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.y:
state: any_identifier '=' expr ';'
{vkd3d_free($1);destroy_block($3);}
- {
$$.lhs_name = $1;$$.rhs_instrs = $3;- }
This isn't where braces are usually aligned; why change them?
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.y:
state_block_start: %empty
{ctx->in_state_block = 1;}
- {
ctx->in_state_block = 1;hlsl_push_scope(ctx);- }
state_block: %empty
- {
if (!($$ = hlsl_alloc(ctx, sizeof(*$$))))YYABORT;- }
Why make it a pointer?
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.y:
vkd3d_free($1);destroy_block($3);}
- {
$$.lhs_name = $1;$$.rhs_instrs = $3;- }
state_block_start: %empty
{ctx->in_state_block = 1;}
- {
ctx->in_state_block = 1;hlsl_push_scope(ctx);
Do we have evidence of a scope here?
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.y:
%type <variable_def> variable_def %type <variable_def> variable_def_typed
+%type <state_block_entry> state
We could just fold the "state" rule into its parent; it's only got one form, and that would simplify things a bit.
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.y:
{ $$ = $1; $$->state_block = $4;
if (TRACE_ON())hlsl_dump_state_block(ctx, $1->name, $4);
Maybe move the TRACE_ON() check to hlsl_dump_state_block()?
Zebediah Figura (@zfigura) commented about libs/vkd3d-shader/hlsl.h:
enum vkd3d_shader_target_type target_type, struct vkd3d_shader_code *out);int hlsl_emit_effect_binary(struct hlsl_ctx *ctx, struct vkd3d_shader_code *out);
+void hlsl_static_constant_folding(struct hlsl_ctx *ctx, struct hlsl_block *expr);
hlsl_fold_static_constants()?
Also, a block isn't really an expr...
I'm not sure about continuing to use a var for the stateblock rhs. Our options are either shoehorning it in somewhere or adding a new IR type. I'm not convinced that LOAD is any better than CONSTANT, though.
On Tue Mar 12 16:21:47 2024 +0000, Francisco Casas wrote:
Still, I think that hlsl_block covers _expression_ and all the other cases. Using node_from_block(), it can be checked if the resulting node is an HLSL_IR_LOAD for the other cases. Even if the "instruction set" is different, it is probably a restrictive subset of the instruction set handled by the hlsl, right?
The problem is that it shouldn't cover all cases, because effect binary contains specific markers, telling which kind of value assignment that is. E.g 0 - constant, 1 - variable, 2 - constant index array access, etc. This is encoded as effect metadata.
Regarding instruction set, it certainly does intersect, but it's not the same. See preshader_ops[] in d3d10/effect.c for known ops. Also there is special way to encode number of used components, and as you can see some instructions do not exist in normal shader language. One option is to have a superset, and then convert them at some point, but to me it seems unfortunate to pollute main compiler logic with marginally used functionality. I don't know what would be the best option.
Also for constant values you can have special predefined constant names like "inv_src_alpha" that are meaningful in context of a state object. We can certainly have all that logic implemented as helpers in fx.c as a number of helpers that would return numeric value for given state object type and constant name.
On Tue Mar 12 16:27:35 2024 +0000, Francisco Casas wrote:
Can you give me or point me to an example of these effect passes? I find it difficult to find, maybe I am not searching properly.
For fx_4+ you can find examples in d3d10/tests/effect.c. The syntax is "technique10 { pass {} }", then pass is what contains a number of entries-state changing "calls" that are applied when pass is applied. All that does is making a number of device context calls as instructed. Same exact thing for fx_2, in d3dx9_36/tests/effect.c, except that it sets individual states.
On Tue Mar 12 17:04:02 2024 +0000, Zebediah Figura wrote:
hlsl_fold_static_constants()? Also, a block isn't really an expr...
Uh, the name is because the first moment I conceived this function was to lower the default values in the rhs of constant buffer members. And I see I had the misconception that all the values in state blocks rhs's have to be constants as well.
Also, a block isn't really an expr...
Yes, but I intended to always pass rhs blocks to this function.
Anyways, I am more inclined now to believe that this function should only be for constant buffer default values and state blocks may require a separate one in fx.c.
The problem is that it shouldn't cover all cases, because effect binary contains specific markers, telling which kind of value assignment that is. E.g 0 - constant, 1 - variable, 2 - constant index array access, etc. This is encoded as effect metadata.
But we could still write a function in fx.c that takes the unmodified block, and identifies the kind of assignment from it, I will try this idea.
Regarding instruction set, it certainly does intersect, but it's not the same. See preshader_ops[] in d3d10/effect.c for known ops. Also there is special way to encode number of used components, and as you can see some instructions do not exist in normal shader language. One option is to have a superset, and then convert them at some point, but to me it seems unfortunate to pollute main compiler logic with marginally used functionality. I don't know what would be the best option.
I don't think we need to add additional instruction opcodes to HLSL IR. The translation to this different set of instructions (the `preshader_ops`) can be contained within fx.c.
We may need though, one or two IR node types, the load of an undefined symbol and maybe the call to an undefined symbol. But that should not interfere with the parsing of other profiles too much.
Also for constant values you can have special predefined constant names like "inv_src_alpha" that are meaningful in context of a state object. We can certainly have all that logic implemented as helpers in fx.c as a number of helpers that would return numeric value for given state object type and constant name.
Okay.
On Tue Mar 12 17:57:55 2024 +0000, Nikolay Sivov wrote:
For fx_4+ you can find examples in d3d10/tests/effect.c. The syntax is "technique10 { pass {} }", then pass is what contains a number of entries-state changing "calls" that are applied when pass is applied. All that does is making a number of device context calls as instructed. Same exact thing for fx_2, in d3dx9_36/tests/effect.c, except that it sets individual states.
Ok, after looking at it, I think it should be used for them too.
We will also need to extend it a little for fx_2_0 passes, for things such as the `compile` keyword: ``` technique tech1 { pass pass1 { VertexShader = compile vs_2_0 main_vs(arg1); } } ```
On Tue Mar 12 17:03:58 2024 +0000, Zebediah Figura wrote:
This isn't where braces are usually aligned; why change them?
Sorry, that was a mistake.
On Tue Mar 12 17:03:58 2024 +0000, Zebediah Figura wrote:
Why make it a pointer?
When I have "complex" structs, that have members via pointers, I have the predisposition to declare them as pointers instead of values.
I tried to rationalize some reasons for it:
- copy-by-value shouldn't be used on them (lest we want "cojoined twins" copies), having them as pointers ensures that I am willingly doing a copy-by-reference or that, if I want a deep copy, I have implemented a function that copies members recursively. - If I ever put values of these structs in an array, I will probably want to make it an array of pointers to these values instead of storing them directly. Two reasons for this: - The cost of moving a pointer is less than the cost of moving a struct that also contains pointers. - If it is a dynamic array, I will have to move them around, breaking external references to these elements if I don't have this extra level of indirection. - Similar to the last one, working with pointers to these values and always allocating these in heap ensures that they will preserve the same position in memory during their whole lifetime, ensuring that external references to it remain valid even if they are passed around. - It feels nice to have a "free" or "cleanup" function that also frees the whole object instead of leaving it with dangling pointers, in case we neglect setting them to NULL. - If makes clear that the owner of the members (and who is responsible their memory) is the object itself and not the object that contains it:
Given ``` struct bar { struct apple *a; struct banana *b; }; ``` consider ``` struct foo { struct bar smol; // Not clear whether "smol" or "big" is the owner of "a" and "b". } big; ``` versus ``` struct foo { // it is clear that only "smol" is the owner of "a" and "b" and it should free their memory. struct bar *smol; } big; ``` Which I think also discourages a little the practice of implicitly transferring ownership.
Even if there are cases where applying this heuristic is not justified, I feel it makes the code more robust for changes that may appear in the future.
Of course, I am willing to revert it when the reviewers don't like it! :)
---
Now I realize that if I were consistent with this rule, I should also have applied this rule to `struct hlsl_state_block_entry` inside `struct hlsl_state_block`, because it contains the `*lhs_name` and `*rhs_instr` pointers, so it is also a "complex" struct. Which may actually make sense if we reuse the struct for technique passes and make it more complex.
On Tue Mar 12 17:04:00 2024 +0000, Zebediah Figura wrote:
Maybe move the TRACE_ON() check to hlsl_dump_state_block()?
I did it this way because we also wrap on `if (TRACE_ON())` other similar calls to ``` // on spirv.c if (TRACE_ON()) rb_for_each_entry(&ctx->functions, dump_function, ctx); ``` ``` // on ir.c and hlsl_codegen.c if (TRACE_ON()) rb_for_each_entry(&ctx->functions, dump_function, ctx); ``` Even though all these functions call `vkd3d_string_buffer_trace()` which also does the check.
On Thu Mar 14 19:30:44 2024 +0000, Zebediah Figura wrote:
I'm not sure about continuing to use a var for the stateblock rhs. Our options are either shoehorning it in somewhere or adding a new IR type. I'm not convinced that LOAD is any better than CONSTANT, though.
Yeah, it sounds more clean to add a new IR type, for identifiers (and possibly function calls) on state blocks that are not identified yet.
On Tue Mar 12 17:03:58 2024 +0000, Zebediah Figura wrote:
Do we have evidence of a scope here?
No, I don't think so. I probably added this to avoid surprised for having `ctx->cur_scope` as `ctx->globals`, but that doesn't seem to be the case.
I am working on a version 2 from the comments I got and now that I feel I have a better understanding of the effects framework.
On Thu Mar 14 19:30:43 2024 +0000, Francisco Casas wrote:
I did it this way because we also wrap on `if (TRACE_ON())` other similar calls to
// on spirv.c if (TRACE_ON()) rb_for_each_entry(&ctx->functions, dump_function, ctx);// on ir.c and hlsl_codegen.c if (TRACE_ON()) rb_for_each_entry(&ctx->functions, dump_function, ctx);Even though all these functions call `vkd3d_string_buffer_trace()` which also does the check.
The point is to avoid doing expensive-looking processing (e.g. iterating a list or rbtree) that doesn't do anything if we aren't ultimately going to trace. But in this case we can just move it to the function body.
I don't care that much though.
- copy-by-value shouldn't be used on them (lest we want "cojoined twins" copies), having them as pointers ensures that I am willingly doing a copy-by-reference or that, if I want a deep copy, I have implemented a function that copies members recursively.
- If I ever put values of these structs in an array, I will probably want to make it an array of pointers to these values instead of storing them directly. Two reasons for this:
- The cost of moving a pointer is less than the cost of moving a struct that also contains pointers.
- If it is a dynamic array, I will have to move them around, breaking external references to these elements if I don't have this extra level of indirection.
- Similar to the last one, working with pointers to these values and always allocating these in heap ensures that they will preserve the same position in memory during their whole lifetime, ensuring that external references to it remain valid even if they are passed around.
I'll admit most of these points seem premature, especially for this structure.
It feels nice to have a "free" or "cleanup" function that also frees the whole object instead of leaving it with dangling pointers, in case we neglect setting them to NULL.
If makes clear that the owner of the members (and who is responsible their memory) is the object itself and not the object that contains it:
Given
struct bar { struct apple *a; struct banana *b; };consider
struct foo { struct bar smol; // Not clear whether "smol" or "big" is the owner of "a" and "b". } big;versus
struct foo { // it is clear that only "smol" is the owner of "a" and "b" and it should free their memory. struct bar *smol; } big;
- Which I think also discourages a little the practice of implicitly transferring ownership.
I don't quite understand this. Idiomatically, in the embedded struct case, big_cleanup() / big_destroy() would call small_cleanup(&big->small) directly, which would itself call apple_destroy(), banana_destroy().
Even if there are cases where applying this heuristic is not justified, I feel it makes the code more robust for changes that may appear in the future.
Of course, I am willing to revert it when the reviewers don't like it! :)
I don't hate it, it just didn't quite seem like the most obvious thing. If you feel strongly about it I don't mind keeping it a pointer.
I don't quite understand this. Idiomatically, in the embedded struct case, big_cleanup() / big_destroy() would call small_cleanup(&big->small) directly, which would itself call apple_destroy(), banana_destroy().
Yes that would be good, but we also may be more tempted to skip the small_cleanup() and instead make `big` manage the memory of big.smol.a and big.smol.b directly, and call apple_destroy(), banana_destroy() directly in small_cleanup(). Arguably, this makes more noise if done through a pointer.
For instance we free `v->arrays.sizes` directly on `free_parse_variable_def(v)`, instead of going through a `free_parse_array_sizes(&v->arrays)` so it is something we have to remember to do if we reuse `v->arrays.sizes` on another structs, or that we have to remember to update if we change `parse_array_sizes` adding other fields that need to be freed.
I am not saying that currently we have to change that, I just wanted to find an example. I understand that there are tradeoffs. Writing more code to prematurely generalize or future-proof is not a good thing (even less for really simple structs like `struct parse_array_sizes`), but I also attempt to avoid having keep track of many places to update when a struct is modified or repurposed.
I don't hate it, it just didn't quite seem like the most obvious thing. If you feel strongly about it I don't mind keeping it a pointer.
No, I also don't feel strongly about it either! In fact in this case I didn't even think it too much. This is just me trying to justify a rule of thumb that I often follow somewhat instinctively.
This merge request was closed by Francisco Casas.
Superseded by !739.
-
Francisco Casas -
Francisco Casas (＠fcasas)
-
Nikolay Sivov (＠nsivov)
-
Zebediah Figura (＠zfigura)