[PATCH v3 2/5] d3dcompiler: Return an instruction list instead of a single instruction from nodes representing expressions.

Matteo Bruni matteo.mystral at gmail.com
Tue Feb 11 11:40:17 CST 2020


On Wed, Feb 5, 2020 at 9:53 PM Zebediah Figura <z.figura12 at gmail.com> wrote:
>
> From: Zebediah Figura <zfigura at codeweavers.com>
>
> Signed-off-by: Zebediah Figura <zfigura at codeweavers.com>
> ---
> v2: don't drop all of the struct initializer elements on the floor
> v3: style tweak, clarify loop_condition()
>
>  dlls/d3dcompiler_43/d3dcompiler_private.h |   6 +
>  dlls/d3dcompiler_43/hlsl.y                | 287 ++++++++++++----------
>  dlls/d3dcompiler_43/utils.c               |  46 +---
>  3 files changed, 165 insertions(+), 174 deletions(-)
>
> diff --git a/dlls/d3dcompiler_43/d3dcompiler_private.h b/dlls/d3dcompiler_43/d3dcompiler_private.h
> index 5fc3161f5f0..9c58830ffe2 100644
> --- a/dlls/d3dcompiler_43/d3dcompiler_private.h
> +++ b/dlls/d3dcompiler_43/d3dcompiler_private.h
> @@ -984,6 +984,7 @@ struct parse_initializer
>  {
>      struct hlsl_ir_node **args;
>      unsigned int args_count;
> +    struct list *instrs;
>  };
>
>  struct parse_variable_def
> @@ -1120,6 +1121,11 @@ static inline struct hlsl_ir_loop *loop_from_node(const struct hlsl_ir_node *nod
>      return CONTAINING_RECORD(node, struct hlsl_ir_loop, node);
>  }
>
> +static inline struct hlsl_ir_node *node_from_list(struct list *list)
> +{
> +    return LIST_ENTRY(list_tail(list), struct hlsl_ir_node, entry);
> +}
> +
>  BOOL add_declaration(struct hlsl_scope *scope, struct hlsl_ir_var *decl, BOOL local_var) DECLSPEC_HIDDEN;
>  struct hlsl_ir_var *get_variable(struct hlsl_scope *scope, const char *name) DECLSPEC_HIDDEN;
>  void free_declaration(struct hlsl_ir_var *decl) DECLSPEC_HIDDEN;
> diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y
> index d81d6afa9cc..f72882187c8 100644
> --- a/dlls/d3dcompiler_43/hlsl.y
> +++ b/dlls/d3dcompiler_43/hlsl.y
> @@ -255,32 +255,31 @@ static void declare_predefined_types(struct hlsl_scope *scope)
>      add_type_to_scope(scope, type);
>  }
>
> -static struct hlsl_ir_if *loop_condition(struct list *cond_list)
> +static BOOL append_conditional_break(struct list *cond_list)
>  {
>      struct hlsl_ir_node *cond, *not_cond;
>      struct hlsl_ir_if *out_cond;
>      struct hlsl_ir_jump *jump;
> -    unsigned int count = list_count(cond_list);
>
> -    if (!count)
> -        return NULL;
> -    if (count != 1)
> -        ERR("Got multiple expressions in a for condition.\n");
> +    /* E.g. "for (i = 0; ; ++i)". */
> +    if (!list_count(cond_list))
> +        return TRUE;
>
> -    cond = LIST_ENTRY(list_head(cond_list), struct hlsl_ir_node, entry);
> +    cond = node_from_list(cond_list);
>      out_cond = d3dcompiler_alloc(sizeof(*out_cond));
>      if (!out_cond)
>      {
>          ERR("Out of memory.\n");
> -        return NULL;
> +        return FALSE;
>      }
>      out_cond->node.type = HLSL_IR_IF;
>      if (!(not_cond = new_unary_expr(HLSL_IR_UNOP_LOGIC_NOT, cond, cond->loc)))
>      {
>          ERR("Out of memory.\n");
>          d3dcompiler_free(out_cond);
> -        return NULL;
> +        return FALSE;
>      }
> +    list_add_tail(cond_list, &not_cond->entry);
>      out_cond->condition = not_cond;
>      jump = d3dcompiler_alloc(sizeof(*jump));
>      if (!jump)
> @@ -288,7 +287,7 @@ static struct hlsl_ir_if *loop_condition(struct list *cond_list)
>          ERR("Out of memory.\n");
>          d3dcompiler_free(out_cond);
>          d3dcompiler_free(not_cond);
> -        return NULL;
> +        return FALSE;
>      }

You shouldn't free not_cond anymore in case of error since you
appended it to cond_list already.

>      jump->node.type = HLSL_IR_JUMP;
>      jump->type = HLSL_IR_JUMP_BREAK;
> @@ -299,12 +298,12 @@ static struct hlsl_ir_if *loop_condition(struct list *cond_list)
>          d3dcompiler_free(out_cond);
>          d3dcompiler_free(not_cond);
>          d3dcompiler_free(jump);
> -        return NULL;
> +        return FALSE;
>      }

Same.

>      list_init(out_cond->then_instrs);
>      list_add_head(out_cond->then_instrs, &jump->node.entry);
>
> -    return out_cond;
> +    return TRUE;

Shouldn't out_cond be inserted somewhere? Right now it looks to me
like it's just leaked.

>  }
>
>  enum loop_type
> @@ -315,7 +314,7 @@ enum loop_type
>  };
>
>  static struct list *create_loop(enum loop_type type, struct list *init, struct list *cond,
> -        struct hlsl_ir_node *iter, struct list *body, struct source_location *loc)
> +        struct list *iter, struct list *body, struct source_location *loc)
>  {
>      struct list *list = NULL;
>      struct hlsl_ir_loop *loop = NULL;
> @@ -340,20 +339,19 @@ static struct list *create_loop(enum loop_type type, struct list *init, struct l
>          goto oom;
>      list_init(loop->body);
>
> -    cond_jump = loop_condition(cond);
> -    if (!cond_jump)
> +    if (!append_conditional_break(cond))
>          goto oom;
>
>      if (type != LOOP_DO_WHILE)
> -        list_add_tail(loop->body, &cond_jump->node.entry);
> +        list_move_tail(loop->body, cond);
>
>      list_move_tail(loop->body, body);
>
>      if (iter)
> -        list_add_tail(loop->body, &iter->entry);
> +        list_move_tail(loop->body, iter);
>
>      if (type == LOOP_DO_WHILE)
> -        list_add_tail(loop->body, &cond_jump->node.entry);
> +        list_move_tail(loop->body, cond);
>
>      d3dcompiler_free(init);
>      d3dcompiler_free(cond);
> @@ -369,7 +367,7 @@ oom:
>      d3dcompiler_free(list);
>      free_instr_list(init);
>      free_instr_list(cond);
> -    free_instr(iter);
> +    free_instr_list(iter);
>      free_instr_list(body);
>      return NULL;
>  }
> @@ -388,9 +386,7 @@ static unsigned int initializer_size(const struct parse_initializer *initializer
>
>  static void free_parse_initializer(struct parse_initializer *initializer)
>  {
> -    unsigned int i;
> -    for (i = 0; i < initializer->args_count; ++i)
> -        free_instr(initializer->args[i]);
> +    free_instr_list(initializer->instrs);
>      d3dcompiler_free(initializer->args);
>  }
>
> @@ -508,6 +504,9 @@ static void struct_var_initializer(struct list *list, struct hlsl_ir_var *var,
>          return;
>      }
>
> +    list_move_tail(list, initializer->instrs);
> +    d3dcompiler_free(initializer->instrs);
> +
>      LIST_FOR_EACH_ENTRY(field, type->e.elements, struct hlsl_struct_field, entry)
>      {
>          struct hlsl_ir_node *node = initializer->args[i];
> @@ -526,6 +525,7 @@ static void struct_var_initializer(struct list *list, struct hlsl_ir_var *var,
>                  break;
>              }
>              deref->node.loc = node->loc;
> +            list_add_tail(list, &deref->node.entry);
>              assignment = make_assignment(&deref->node, ASSIGN_OP_ASSIGN, BWRITERSP_WRITEMASK_ALL, node);
>              list_add_tail(list, &assignment->entry);
>          }
> @@ -533,9 +533,6 @@ static void struct_var_initializer(struct list *list, struct hlsl_ir_var *var,
>              FIXME("Initializing with \"mismatched\" fields is not supported yet.\n");
>      }
>
> -    /* Free initializer elements in excess. */
> -    for (; i < initializer->args_count; ++i)
> -        free_instr(initializer->args[i]);
>      d3dcompiler_free(initializer->args);
>  }
>
> @@ -609,6 +606,7 @@ 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_deref *deref;
>
>              TRACE("Variable with initializer.\n");
>              if (type->type <= HLSL_CLASS_LAST_NUMERIC
> @@ -661,7 +659,10 @@ static struct list *declare_vars(struct hlsl_type *basic_type, DWORD modifiers,
>                  continue;
>              }
>
> -            assignment = make_assignment(&new_var_deref(var)->node, ASSIGN_OP_ASSIGN,
> +            deref = new_var_deref(var);
> +            list_add_tail(statements_list, &deref->node.entry);
> +            list_add_tail(statements_list, &v->initializer.args[0]->entry);
> +            assignment = make_assignment(&deref->node, ASSIGN_OP_ASSIGN,
>                      BWRITERSP_WRITEMASK_ALL, v->initializer.args[0]);
>              d3dcompiler_free(v->initializer.args);
>              list_add_tail(statements_list, &assignment->entry);
> @@ -880,6 +881,35 @@ static const struct hlsl_ir_function_decl *get_overloaded_func(struct wine_rb_tr
>      return NULL;
>  }
>
> +static struct list *append_unop(struct list *list, struct hlsl_ir_node *node)
> +{
> +    list_add_tail(list, &node->entry);
> +    return list;
> +}
> +
> +static struct list *append_binop(struct list *first, struct list *second, struct hlsl_ir_node *node)
> +{
> +    list_move_tail(first, second);
> +    d3dcompiler_free(second);
> +    list_add_tail(first, &node->entry);
> +    return first;
> +}
> +
> +static struct list *make_list(struct hlsl_ir_node *node)
> +{
> +    struct list *list;
> +
> +    if (!(list = d3dcompiler_alloc(sizeof(*list))))
> +    {
> +        ERR("Out of memory.\n");
> +        free_instr(node);
> +        return NULL;
> +    }
> +    list_init(list);
> +    list_add_tail(list, &node->entry);
> +    return list;
> +}
> +
>  %}
>
>  %locations
> @@ -1024,12 +1054,12 @@ static const struct hlsl_ir_function_decl *get_overloaded_func(struct wine_rb_tr
>  %type <variable_def> type_spec
>  %type <initializer> complex_initializer
>  %type <initializer> initializer_expr_list
> -%type <instr> initializer_expr
> +%type <list> initializer_expr
>  %type <modifiers> var_modifiers
>  %type <list> field
>  %type <list> parameters
>  %type <list> param_list
> -%type <instr> expr
> +%type <list> expr
>  %type <intval> array
>  %type <list> statement
>  %type <list> statement_list
> @@ -1048,21 +1078,21 @@ static const struct hlsl_ir_function_decl *get_overloaded_func(struct wine_rb_tr
>  %type <list> variables_def
>  %type <list> variables_def_optional
>  %type <if_body> if_body
> -%type <instr> primary_expr
> -%type <instr> postfix_expr
> -%type <instr> unary_expr
> -%type <instr> mul_expr
> -%type <instr> add_expr
> -%type <instr> shift_expr
> -%type <instr> relational_expr
> -%type <instr> equality_expr
> -%type <instr> bitand_expr
> -%type <instr> bitxor_expr
> -%type <instr> bitor_expr
> -%type <instr> logicand_expr
> -%type <instr> logicor_expr
> -%type <instr> conditional_expr
> -%type <instr> assignment_expr
> +%type <list> primary_expr
> +%type <list> postfix_expr
> +%type <list> unary_expr
> +%type <list> mul_expr
> +%type <list> add_expr
> +%type <list> shift_expr
> +%type <list> relational_expr
> +%type <list> equality_expr
> +%type <list> bitand_expr
> +%type <list> bitxor_expr
> +%type <list> bitor_expr
> +%type <list> logicand_expr
> +%type <list> logicor_expr
> +%type <list> conditional_expr
> +%type <list> assignment_expr
>  %type <list> expr_statement
>  %type <unary_op> unary_op
>  %type <assign_op> assign_op
> @@ -1615,7 +1645,7 @@ array:                    /* Empty */
>                              {
>                                  FIXME("Array.\n");
>                                  $$ = 0;
> -                                free_instr($2);
> +                                free_instr_list($2);
>                              }
>
>  var_modifiers:            /* Empty */
> @@ -1672,7 +1702,8 @@ complex_initializer:      initializer_expr
>                                  $$.args_count = 1;
>                                  if (!($$.args = d3dcompiler_alloc(sizeof(*$$.args))))
>                                      YYABORT;
> -                                $$.args[0] = $1;
> +                                $$.args[0] = node_from_list($1);
> +                                $$.instrs = $1;
>                              }
>                          | '{' initializer_expr_list '}'
>                              {
> @@ -1693,14 +1724,17 @@ initializer_expr_list:    initializer_expr
>                                  $$.args_count = 1;
>                                  if (!($$.args = d3dcompiler_alloc(sizeof(*$$.args))))
>                                      YYABORT;
> -                                $$.args[0] = $1;
> +                                $$.args[0] = node_from_list($1);
> +                                $$.instrs = $1;
>                              }
>                          | initializer_expr_list ',' initializer_expr
>                              {
>                                  $$ = $1;
>                                  if (!($$.args = d3dcompiler_realloc($$.args, ($$.args_count + 1) * sizeof(*$$.args))))
>                                      YYABORT;
> -                                $$.args[$$.args_count++] = $3;
> +                                $$.args[$$.args_count++] = node_from_list($3);
> +                                list_move_tail($$.instrs, $3);
> +                                d3dcompiler_free($3);
>                              }
>
>  boolean:                  KW_TRUE
> @@ -1742,16 +1776,14 @@ jump_statement:           KW_RETURN expr ';'
>                                  jump->node.type = HLSL_IR_JUMP;
>                                  set_location(&jump->node.loc, &@1);
>                                  jump->type = HLSL_IR_JUMP_RETURN;
> -                                jump->node.data_type = $2->data_type;
> -                                jump->return_value = $2;
> +                                jump->node.data_type = node_from_list($2)->data_type;
> +                                jump->return_value = node_from_list($2);
>
>                                  FIXME("Check for valued return on void function.\n");
>                                  FIXME("Implicit conversion to the return type if needed, "
>                                         "error out if conversion not possible.\n");
>
> -                                $$ = d3dcompiler_alloc(sizeof(*$$));
> -                                list_init($$);
> -                                list_add_tail($$, &jump->node.entry);
> +                                $$ = append_unop($2, &jump->node);

I'm a bit thrown off by the use of append_unop() here (and for the
"if" below): jump technically isn't a unary operator (expression).
Maybe just do an explicit list_add_tail() like the previous code?
I don't think you have to necessarily take this suggestion, I suspect
I might not get the same uneasiness next time I read the patch...

>                              }
>
>  selection_statement:      KW_IF '(' expr ')' if_body
> @@ -1764,18 +1796,16 @@ selection_statement:      KW_IF '(' expr ')' if_body
>                                  }
>                                  instr->node.type = HLSL_IR_IF;
>                                  set_location(&instr->node.loc, &@1);
> -                                instr->condition = $3;
> +                                instr->condition = node_from_list($3);
>                                  instr->then_instrs = $5.then_instrs;
>                                  instr->else_instrs = $5.else_instrs;
> -                                if ($3->data_type->dimx > 1 || $3->data_type->dimy > 1)
> +                                if (instr->condition->data_type->dimx > 1 || instr->condition->data_type->dimy > 1)
>                                  {
>                                      hlsl_report_message(instr->node.loc.file, instr->node.loc.line,
>                                              instr->node.loc.col, HLSL_LEVEL_ERROR,
>                                              "if condition requires a scalar");
>                                  }
> -                                $$ = d3dcompiler_alloc(sizeof(*$$));
> -                                list_init($$);
> -                                list_add_head($$, &instr->node.entry);
> +                                $$ = append_unop($3, &instr->node);
>                              }
>
>  if_body:                  statement
> @@ -1792,32 +1822,14 @@ if_body:                  statement
>  loop_statement:           KW_WHILE '(' expr ')' statement
>                              {
>                                  struct source_location loc;
> -                                struct list *cond = d3dcompiler_alloc(sizeof(*cond));
> -
> -                                if (!cond)
> -                                {
> -                                    ERR("Out of memory.\n");
> -                                    YYABORT;
> -                                }
> -                                list_init(cond);
> -                                list_add_head(cond, &$3->entry);
>                                  set_location(&loc, &@1);
> -                                $$ = create_loop(LOOP_WHILE, NULL, cond, NULL, $5, &loc);
> +                                $$ = create_loop(LOOP_WHILE, NULL, $3, NULL, $5, &loc);
>                              }
>                          | KW_DO statement KW_WHILE '(' expr ')' ';'
>                              {
>                                  struct source_location loc;
> -                                struct list *cond = d3dcompiler_alloc(sizeof(*cond));
> -
> -                                if (!cond)
> -                                {
> -                                    ERR("Out of memory.\n");
> -                                    YYABORT;
> -                                }
> -                                list_init(cond);
> -                                list_add_head(cond, &$5->entry);
>                                  set_location(&loc, &@1);
> -                                $$ = create_loop(LOOP_DO_WHILE, NULL, cond, NULL, $2, &loc);
> +                                $$ = create_loop(LOOP_DO_WHILE, NULL, $5, NULL, $2, &loc);
>                              }
>                          | KW_FOR '(' scope_start expr_statement expr_statement expr ')' statement
>                              {
> @@ -1846,10 +1858,7 @@ expr_statement:           ';'
>                              }
>                          | expr ';'
>                              {
> -                                $$ = d3dcompiler_alloc(sizeof(*$$));
> -                                list_init($$);
> -                                if ($1)
> -                                    list_add_head($$, &$1->entry);
> +                                $$ = $1;
>                              }
>
>  primary_expr:             C_FLOAT
> @@ -1864,7 +1873,8 @@ primary_expr:             C_FLOAT
>                                  set_location(&c->node.loc, &yylloc);
>                                  c->node.data_type = new_hlsl_type(d3dcompiler_strdup("float"), HLSL_CLASS_SCALAR, HLSL_TYPE_FLOAT, 1, 1);
>                                  c->v.value.f[0] = $1;
> -                                $$ = &c->node;
> +                                if (!($$ = make_list(&c->node)))
> +                                    YYABORT;
>                              }
>                          | C_INTEGER
>                              {
> @@ -1878,7 +1888,8 @@ primary_expr:             C_FLOAT
>                                  set_location(&c->node.loc, &yylloc);
>                                  c->node.data_type = new_hlsl_type(d3dcompiler_strdup("int"), HLSL_CLASS_SCALAR, HLSL_TYPE_INT, 1, 1);
>                                  c->v.value.i[0] = $1;
> -                                $$ = &c->node;
> +                                if (!($$ = make_list(&c->node)))
> +                                    YYABORT;
>                              }
>                          | boolean
>                              {
> @@ -1892,7 +1903,8 @@ primary_expr:             C_FLOAT
>                                  set_location(&c->node.loc, &yylloc);
>                                  c->node.data_type = new_hlsl_type(d3dcompiler_strdup("bool"), HLSL_CLASS_SCALAR, HLSL_TYPE_BOOL, 1, 1);
>                                  c->v.value.b[0] = $1;
> -                                $$ = &c->node;
> +                                if (!($$ = make_list(&c->node)))
> +                                    YYABORT;
>                              }
>                          | VAR_IDENTIFIER
>                              {
> @@ -1908,8 +1920,9 @@ primary_expr:             C_FLOAT
>                                  }
>                                  if ((deref = new_var_deref(var)))
>                                  {
> -                                    $$ = &deref->node;
> -                                    set_location(&$$->loc, &@1);
> +                                    set_location(&deref->node.loc, &@1);
> +                                    if (!($$ = make_list(&deref->node)))
> +                                        YYABORT;
>                                  }
>                                  else
>                                      $$ = NULL;
> @@ -1926,43 +1939,47 @@ postfix_expr:             primary_expr
>                          | postfix_expr OP_INC
>                              {
>                                  struct source_location loc;
> +                                struct hlsl_ir_node *inc;
>
>                                  set_location(&loc, &@2);
> -                                if ($1->data_type->modifiers & HLSL_MODIFIER_CONST)
> +                                if (node_from_list($1)->data_type->modifiers & HLSL_MODIFIER_CONST)
>                                  {
>                                      hlsl_report_message(loc.file, loc.line, loc.col, HLSL_LEVEL_ERROR,
>                                              "modifying a const expression");
>                                      YYABORT;
>                                  }
> -                                $$ = new_unary_expr(HLSL_IR_UNOP_POSTINC, $1, loc);
> +                                inc = new_unary_expr(HLSL_IR_UNOP_POSTINC, node_from_list($1), loc);
>                                  /* Post increment/decrement expressions are considered const */
> -                                $$->data_type = clone_hlsl_type($$->data_type);
> -                                $$->data_type->modifiers |= HLSL_MODIFIER_CONST;
> +                                inc->data_type = clone_hlsl_type(inc->data_type);
> +                                inc->data_type->modifiers |= HLSL_MODIFIER_CONST;
> +                                $$ = append_unop($1, inc);
>                              }
>                          | postfix_expr OP_DEC
>                              {
>                                  struct source_location loc;
> +                                struct hlsl_ir_node *inc;
>
>                                  set_location(&loc, &@2);
> -                                if ($1->data_type->modifiers & HLSL_MODIFIER_CONST)
> +                                if (node_from_list($1)->data_type->modifiers & HLSL_MODIFIER_CONST)
>                                  {
>                                      hlsl_report_message(loc.file, loc.line, loc.col, HLSL_LEVEL_ERROR,
>                                              "modifying a const expression");
>                                      YYABORT;
>                                  }
> -                                $$ = new_unary_expr(HLSL_IR_UNOP_POSTDEC, $1, loc);
> +                                inc = new_unary_expr(HLSL_IR_UNOP_POSTDEC, node_from_list($1), loc);
>                                  /* Post increment/decrement expressions are considered const */
> -                                $$->data_type = clone_hlsl_type($$->data_type);
> -                                $$->data_type->modifiers |= HLSL_MODIFIER_CONST;
> +                                inc->data_type = clone_hlsl_type(inc->data_type);
> +                                inc->data_type->modifiers |= HLSL_MODIFIER_CONST;
> +                                $$ = append_unop($1, inc);
>                              }
>                          | postfix_expr '.' any_identifier
>                              {
>                                  struct source_location loc;
>
>                                  set_location(&loc, &@2);
> -                                if ($1->data_type->type == HLSL_CLASS_STRUCT)
> +                                if (node_from_list($1)->data_type->type == HLSL_CLASS_STRUCT)
>                                  {
> -                                    struct hlsl_type *type = $1->data_type;
> +                                    struct hlsl_type *type = node_from_list($1)->data_type;
>                                      struct hlsl_struct_field *field;
>
>                                      $$ = NULL;

It might be nicer to store the result of node_from_list($1) in a local
variable and use it through.



More information about the wine-devel mailing list