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

[Zebediah Figura]

Six months later...

On 8/21/19 12:41 PM, Matteo Bruni wrote:
> On Tue, Aug 13, 2019 at 10:55 PM Zebediah Figura
> <zfigura at codeweavers.com> wrote:
>>
>> Signed-off-by: Zebediah Figura <zfigura at codeweavers.com>
>> ---
>> 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 *.

No, because we want to append the whole condition expression (i.e. the
list containing all of the condition nodes as well as the branch/jump.)
"cond_jump" is already tacked onto the end of "cond" by
loop_condition(). I will agree these functions are not particularly
clear; we could probably do better by returning BOOL from
loop_condition() and renaming it to something like
make_conditional_break(). I'll make that change in v3.

> 
>>      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?

Yep, done.

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