From: Zebediah Figura Subject: Re: [PATCH 5/6] d3dcompiler: Store initializers inside the hlsl_ir_var structure. Message-Id: Date: Wed, 17 Jun 2020 10:47:01 -0500 In-Reply-To: References: <20200611214422.275678-1-zfigura@codeweavers.com> <20200611214422.275678-5-zfigura@codeweavers.com> On 6/17/20 10:29 AM, Matteo Bruni wrote: > On Thu, Jun 11, 2020 at 11:45 PM Zebediah Figura wrote: >> >> This is necessary so that global variable initializers can work correctly (both static and extern). >> >> Signed-off-by: Zebediah Figura >> --- >> dlls/d3dcompiler_43/d3dcompiler_private.h | 1 + >> dlls/d3dcompiler_43/hlsl.y | 155 ++++++++++++---------- >> dlls/d3dcompiler_43/utils.c | 4 + >> 3 files changed, 93 insertions(+), 67 deletions(-) >> >> diff --git a/dlls/d3dcompiler_43/d3dcompiler_private.h b/dlls/d3dcompiler_43/d3dcompiler_private.h >> index 9feaee0d8ac..bc8734f0148 100644 >> --- a/dlls/d3dcompiler_43/d3dcompiler_private.h >> +++ b/dlls/d3dcompiler_43/d3dcompiler_private.h >> @@ -706,6 +706,7 @@ struct hlsl_ir_var >> unsigned int modifiers; >> const struct reg_reservation *reg_reservation; >> struct list scope_entry, param_entry; >> + struct list initializer; >> >> unsigned int first_write, last_read; >> }; >> diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y >> index 393f8d52fbd..734289f66a5 100644 >> --- a/dlls/d3dcompiler_43/hlsl.y >> +++ b/dlls/d3dcompiler_43/hlsl.y >> @@ -525,6 +525,7 @@ static struct hlsl_ir_var *new_var(const char *name, struct hlsl_type *type, con >> var->semantic = semantic; >> var->modifiers = modifiers; >> var->reg_reservation = reg_reservation; >> + list_init(&var->initializer); >> return var; >> } >> >> @@ -770,30 +771,18 @@ static void free_parse_variable_def(struct parse_variable_def *v) >> d3dcompiler_free(v); >> } >> >> -static struct list *declare_vars(struct hlsl_type *basic_type, DWORD modifiers, struct list *var_list) >> +static BOOL declare_vars(struct hlsl_type *basic_type, DWORD modifiers, struct list *var_list) >> { >> struct hlsl_type *type; >> struct parse_variable_def *v, *v_next; >> struct hlsl_ir_var *var; >> - struct hlsl_ir_node *assignment; >> BOOL ret, local = TRUE; >> - struct list *statements_list = d3dcompiler_alloc(sizeof(*statements_list)); >> >> if (basic_type->type == HLSL_CLASS_MATRIX) >> assert(basic_type->modifiers & HLSL_MODIFIERS_MAJORITY_MASK); >> >> - if (!statements_list) >> - { >> - ERR("Out of memory.\n"); >> - LIST_FOR_EACH_ENTRY_SAFE(v, v_next, var_list, struct parse_variable_def, entry) >> - free_parse_variable_def(v); >> - d3dcompiler_free(var_list); >> - return NULL; >> - } >> - list_init(statements_list); >> - >> if (!var_list) >> - return statements_list; >> + return TRUE; >> >> LIST_FOR_EACH_ENTRY_SAFE(v, v_next, var_list, struct parse_variable_def, entry) >> { >> @@ -808,6 +797,7 @@ static struct list *declare_vars(struct hlsl_type *basic_type, DWORD modifiers, >> continue; >> } >> debug_dump_decl(type, modifiers, v->name, v->loc.line); >> + list_init(&var->initializer); >> >> if (hlsl_ctx.cur_scope == hlsl_ctx.globals) >> { >> @@ -835,7 +825,6 @@ static struct list *declare_vars(struct hlsl_type *basic_type, DWORD modifiers, >> if (v->initializer.args_count) >> { >> unsigned int size = initializer_size(&v->initializer); >> - struct hlsl_ir_load *load; >> >> TRACE("Variable with initializer.\n"); >> if (type->type <= HLSL_CLASS_LAST_NUMERIC >> @@ -862,7 +851,7 @@ static struct list *declare_vars(struct hlsl_type *basic_type, DWORD modifiers, >> >> if (type->type == HLSL_CLASS_STRUCT) >> { >> - struct_var_initializer(statements_list, var, &v->initializer); >> + struct_var_initializer(&var->initializer, var, &v->initializer); >> d3dcompiler_free(v); >> continue; >> } >> @@ -888,19 +877,16 @@ static struct list *declare_vars(struct hlsl_type *basic_type, DWORD modifiers, >> continue; >> } >> >> - list_move_tail(statements_list, v->initializer.instrs); >> + list_move_tail(&var->initializer, v->initializer.instrs); >> d3dcompiler_free(v->initializer.instrs); >> - >> - load = new_var_load(var, var->loc); >> - list_add_tail(statements_list, &load->node.entry); >> - assignment = make_assignment(&load->node, ASSIGN_OP_ASSIGN, v->initializer.args[0]); >> d3dcompiler_free(v->initializer.args); >> - list_add_tail(statements_list, &assignment->entry); >> + >> + implicit_conversion(node_from_list(&var->initializer), var->data_type, &var->loc); >> } >> d3dcompiler_free(v); >> } >> d3dcompiler_free(var_list); >> - return statements_list; >> + return TRUE; >> } >> >> static BOOL add_struct_field(struct list *fields, struct hlsl_struct_field *field) >> @@ -1282,6 +1268,20 @@ static struct hlsl_ir_function_decl *new_func_decl(struct hlsl_type *return_type >> return decl; >> } >> >> +/* Append an instruction assigning "var" to its own initializer. */ >> +static BOOL add_assignment_to_initializer(struct hlsl_ir_var *var) >> +{ >> + struct hlsl_ir_assignment *assign; >> + >> + if (!list_empty(&var->initializer)) >> + { >> + if (!(assign = make_simple_assignment(var, node_from_list(&var->initializer)))) >> + return FALSE; >> + list_add_tail(&var->initializer, &assign->node.entry); >> + } >> + return TRUE; >> +} >> + >> %} >> >> %locations >> @@ -1416,9 +1416,6 @@ static struct hlsl_ir_function_decl *new_func_decl(struct hlsl_type *return_type >> %type boolean >> %type base_type >> %type type >> -%type declaration_statement >> -%type declaration >> -%type struct_declaration >> %type struct_spec >> %type named_struct_spec >> %type unnamed_struct_spec >> @@ -1566,7 +1563,8 @@ struct_declaration: var_modifiers struct_spec variables_def_optional ';' >> >> if (!(type = apply_type_modifiers($2, &modifiers, get_location(&@1)))) >> YYABORT; >> - $$ = declare_vars(type, modifiers, $3); >> + if (!declare_vars(type, modifiers, $3)) >> + YYABORT; >> } >> >> struct_spec: named_struct_spec >> @@ -1908,18 +1906,11 @@ base_type: >> d3dcompiler_free($2); >> } >> >> -declaration_statement: declaration >> - | struct_declaration >> - | typedef >> - { >> - $$ = d3dcompiler_alloc(sizeof(*$$)); >> - if (!$$) >> - { >> - ERR("Out of memory\n"); >> - YYABORT; >> - } >> - list_init($$); >> - } >> +declaration_statement: >> + >> + declaration >> + | struct_declaration >> + | typedef >> >> typedef_type: type >> | struct_spec >> @@ -1967,7 +1958,8 @@ declaration: var_modifiers type variables_def ';' >> >> if (!(type = apply_type_modifiers($2, &modifiers, get_location(&@1)))) >> YYABORT; >> - $$ = declare_vars(type, modifiers, $3); >> + if (!declare_vars(type, modifiers, $3)) >> + YYABORT; >> } >> >> variables_def_optional: /* Empty */ >> @@ -2148,12 +2140,29 @@ statement_list: statement >> d3dcompiler_free($2); >> } >> >> -statement: declaration_statement >> - | expr_statement >> - | compound_statement >> - | jump_statement >> - | selection_statement >> - | loop_statement >> +statement: >> + >> + declaration_statement >> + { >> + struct hlsl_ir_var *var; >> + >> + /* Append any initializers made by this declaration_statement to the >> + * instruction list. */ >> + if (!($$ = d3dcompiler_alloc(sizeof(*$$)))) >> + YYABORT; >> + list_init($$); >> + LIST_FOR_EACH_ENTRY(var, &hlsl_ctx.cur_scope->vars, struct hlsl_ir_var, scope_entry) >> + { >> + if (!add_assignment_to_initializer(var)) >> + YYABORT; >> + list_move_tail($$, &var->initializer); >> + } >> + } > > I see what you're doing but I'm a bit bothered by the part where you > store the initializers into struct hlsl_ir_var. I don't know that it > is a fair "complaint", it's certainly mostly stylistical. But hear my > variation and see if it makes any sense to you. > > Initializers could be temporarily stored in a special list inside > hlsl_ctx. Here in declaration_statement and other places where you > process initializers, you list_move_head() from that list to $$ and > that's it. It would work, I think, and I agree it'd be simpler than this patch. The one place this approach gets hairy is with uniform initializers (which translate to default values). We could parse that in declare_vars() and store it as a flat value in hlsl_ir_var without difficulty, but we'd need to be able to run constant folding over it (and maybe other passes? Of course, I don't even have examples of anything that needs uniform initializers.) > Local static initializers should probably be stored into a > separate list, together with global initializers (assuming static > locals are a thing in HLSL, I don't recall off the top of my head). Yes, and they act like C static locals, which is to say, like static globals. > >> + | expr_statement >> + | compound_statement >> + | jump_statement >> + | selection_statement >> + | loop_statement >> >> jump_statement: KW_RETURN expr ';' >> { >> @@ -2206,27 +2215,39 @@ if_body: statement >> $$.else_instrs = $3; >> } >> >> -loop_statement: KW_WHILE '(' expr ')' statement >> - { >> - $$ = create_loop(LOOP_WHILE, NULL, $3, NULL, $5, get_location(&@1)); >> - } >> - | KW_DO statement KW_WHILE '(' expr ')' ';' >> - { >> - $$ = create_loop(LOOP_DO_WHILE, NULL, $5, NULL, $2, get_location(&@1)); >> - } >> - | KW_FOR '(' scope_start expr_statement expr_statement expr ')' statement >> - { >> - $$ = create_loop(LOOP_FOR, $4, $5, $6, $8, get_location(&@1)); >> - pop_scope(&hlsl_ctx); >> - } >> - | KW_FOR '(' scope_start declaration expr_statement expr ')' statement >> - { >> - if (!$4) >> - hlsl_report_message(get_location(&@4), HLSL_LEVEL_WARNING, >> - "no expressions in for loop initializer"); >> - $$ = create_loop(LOOP_FOR, $4, $5, $6, $8, get_location(&@1)); >> - pop_scope(&hlsl_ctx); >> - } >> +loop_statement: >> + >> + KW_WHILE '(' expr ')' statement >> + { >> + $$ = create_loop(LOOP_WHILE, NULL, $3, NULL, $5, get_location(&@1)); >> + } >> + | KW_DO statement KW_WHILE '(' expr ')' ';' >> + { >> + $$ = create_loop(LOOP_DO_WHILE, NULL, $5, NULL, $2, get_location(&@1)); >> + } >> + | KW_FOR '(' scope_start expr_statement expr_statement expr ')' statement >> + { >> + $$ = create_loop(LOOP_FOR, $4, $5, $6, $8, get_location(&@1)); >> + pop_scope(&hlsl_ctx); >> + } >> + | KW_FOR '(' scope_start declaration expr_statement expr ')' statement >> + { >> + struct hlsl_ir_var *var; >> + >> + $$ = create_loop(LOOP_FOR, NULL, $5, $6, $8, get_location(&@1)); >> + >> + /* Prepend any initializers made by this declaration to the >> + * instruction list. Walk through the list in reverse, so that the >> + * initializers are executed in the order that they were declared. */ >> + LIST_FOR_EACH_ENTRY_REV(var, &hlsl_ctx.cur_scope->vars, struct hlsl_ir_var, scope_entry) >> + { >> + if (!add_assignment_to_initializer(var)) >> + YYABORT; >> + list_move_head($$, &var->initializer); >> + } > > If I'm reading this correctly, here the temporary list idea would > avoid the need to go in reverse, which might be a bit less surprising > overall. > -----BEGIN PGP SIGNATURE----- iQEzBAEBCAAdFiEENTo75Twe9rbPART3DZ01igeheEAFAl7qOvUACgkQDZ01igeh eEAFfAgApPf32tYdk2XV0WFO2dZLYnlv3ZBnKKh8ZdtRy0mMvG+9lM2vIJ4KPxj6 anKmxLdQsYYAlQCCnMsZR2I/7a9pI64xdloDWcDqM8a4DOndfjILNzTVtkPqZ7+k p7UXhwJfi72mL2RXf21V7Vp2nOjgzfnjqWJdBvB+adu08LhonFVPSuXX81al0Wc1 lY6ZKnqL3e0VGVGLF+/Qh4ApE0XYjE5G+QlBmc6iVenE4wY0bmxuM16x/6F92BXf 54ZU0lZPOqZhhMqVPAQ3mmSWUwGEzq3MpPfCHSMwx0T96qm3B1figCY4xR/wumVi RJ4mS+0laPf46tWr7F1j1qzPd0K/kQ== =xYS5 -----END PGP SIGNATURE-----