Re: [PATCH v5] d3dcompiler: Return an instruction list instead of a single instruction from nodes representing expressions.

[Matteo Bruni]

On Thu, Feb 13, 2020 at 6:01 AM Zebediah Figura  wrote:
> From: Zebediah Figura
> Signed-off-by: Zebediah Figura
>
> v2: don't drop all of the struct initializer elements on the floor
> v3: style tweak, clarify loop_condition()
> v4: fix append_conditional_break(); remove confusing uses of append_unop()
> v5: don't drop all of the variable initializer instructions on the floor

Patch looks great! There should be a separate signoff email on the
way, but it also made me think about a few random things WRT the
compiler in general. Nothing to necessarly act on. Anyway, a couple of
comments inline...
> -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_node *condition, *not;
> struct hlsl_ir_jump *jump;
> - unsigned int count = list_count(cond_list);
> - struct hlsl_ir_if *iff;
> - 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);
> - out_cond = d3dcompiler_alloc(sizeof(*out_cond));
> - if (!out_cond)
> - condition = node_from_list(cond_list);
> - if (!(not = new_unary_expr(HLSL_IR_UNOP_LOGIC_NOT, condition, condition->loc)))
> {
>     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)))
> - list_add_tail(cond_list, &not->entry);
> -
> - if (!(iff = d3dcompiler_alloc(sizeof(*iff))))
> {
>     ERR("Out of memory.\n");
> -
>    d3dcompiler_free(out_cond);
> -
>    return NULL;
> -
>    return FALSE;
> }
> - out_cond->condition = not_cond;
> - jump = d3dcompiler_alloc(sizeof(*jump));
> - if (!jump)
> - iff->node.type = HLSL_IR_IF;
> - iff->condition = not;
> - list_add_tail(cond_list, &iff->node.entry);
> -
> - if (!(iff->then_instrs = d3dcompiler_alloc(sizeof(*iff->then_instrs))))
> {
>     ERR("Out of memory.\n");
> -
>    d3dcompiler_free(out_cond);
> -
>    d3dcompiler_free(not_cond);
> -
>    return NULL;
> -
>    return FALSE;
> }
> - jump->node.type = HLSL_IR_JUMP;
> - jump->type = HLSL_IR_JUMP_BREAK;
> - out_cond->then_instrs = d3dcompiler_alloc(sizeof(*out_cond->then_instrs));
> - if (!out_cond->then_instrs)
> - list_init(iff->then_instrs);

then_instrs and else_instrs lists could be stored inline in struct
hlsl_ir_jump. We'd need to be more careful when copying or moving the
instruction around though.
> -
> - if (!(jump = d3dcompiler_alloc(sizeof(*jump))))
> {
>     ERR("Out of memory.\n");
> -
>    d3dcompiler_free(out_cond);
> -
>    d3dcompiler_free(not_cond);
> -
>    d3dcompiler_free(jump);
> -
>    return NULL;
> -
>    return FALSE;
> }
> - list_init(out_cond->then_instrs);
> - list_add_head(out_cond->then_instrs, &jump->node.entry);
> -
> - return out_cond;
> - jump->node.type = HLSL_IR_JUMP;
> - jump->type = HLSL_IR_JUMP_BREAK;
> - list_add_head(iff->then_instrs, &jump->node.entry);
> - return TRUE;
>
> }

Something unrelated to the patch that occurred to me while reading
this. It seems like it might be useful to have "builder" functions
(one for each instruction type) that allocate an instruction and
initialize it with the relevant values.
I suspect though that I tried something like that in the past and
didn't like the end result too much so maybe not. I guess what I'm
saying is, keep it in mind going forward and see if it makes sense.

BTW, one nice (if mostly incidental) thing about the switch to a flat
IR is clear here: you don't need to release the intermediate
instructions on error.
> @@ -1764,18 +1793,17 @@ 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);
> -
>                            $$ = $3;
> -
>                            list_add_tail($$, &instr->node.entry);
>                        }

We could have then_instrs and else_instrs (and the likes in the other
control flow instructions) be "basic block" structures instead of
plain lists, with room for additional data. Not sure how useful it
would be in practice, you probably have a better idea by now.