Re: [PATCH v2 3/4] d3dcompiler: Return an instruction list instead of a single instruction from nodes representing expressions.

[Matteo Bruni]

On Tue, Aug 13, 2019 at 10:55 PM Zebediah Figura
 wrote:
> Signed-off-by: Zebediah Figura
>
> v2: don't drop all of the struct initializer elements on the floor
>
> dlls/d3dcompiler_43/d3dcompiler_private.h |   6 +
>  dlls/d3dcompiler_43/hlsl.y                | 269 ++++++++++++----------
>  dlls/d3dcompiler_43/utils.c               |  45 +---
>  3 files changed, 155 insertions(+), 165 deletions(-)
> @@ -315,7 +313,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;
> @@ -345,15 +343,15 @@ static struct list *create_loop(enum loop_type type, struct list *init, struct l
>          goto oom;
>  if (type != LOOP_DO_WHILE)
> -
>    list_add_tail(loop->body, &cond_jump->node.entry);
> -
>    list_move_tail(loop->body, cond);

Should this use cond_jump? Right now cond_jump seems unused and cond
is freed at the end of the function.
FWIW cond_jump is really a misnomer, it's a struct hlsl_ir_if *.
>  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);

Same.
> diff --git a/dlls/d3dcompiler_43/utils.c b/dlls/d3dcompiler_43/utils.c
> index 1dea5ba5bf8..8f069b17ec7 100644
> --- a/dlls/d3dcompiler_43/utils.c
> +++ b/dlls/d3dcompiler_43/utils.c
> @@ -1277,10 +1277,13 @@ static struct hlsl_type *expr_common_type(struct hlsl_type *t1, struct hlsl_type
>  static struct hlsl_ir_node *implicit_conversion(struct hlsl_ir_node *node, struct hlsl_type *type,
>          struct source_location *loc)
>  {
> - struct hlsl_ir_expr *cast;
> if (compare_hlsl_types(node->data_type, type))
>     return node;
> TRACE("Implicit conversion of expression to %s\n", debug_hlsl_type(type));
> - return &new_cast(node, type, loc)->node;
> - if ((cast = new_cast(node, type, loc)))
> -
>    list_add_after(&node->entry, &cast->node.entry);
> - return &cast->node;
>
> }

Nitpick, can you leave a blank line between the declarations and the code?

Aside from those points, I like the patch and the IR flattening in
particular. Nice job!