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

[Zebediah Figura]

On 2/14/20 12:11 PM, Matteo Bruni wrote:
> On Thu, Feb 13, 2020 at 6:01 AM 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()
>> 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.

I think you're right, yes.

> 
>> +
>> +    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.

Yeah, I tend to prefer that approach. It's not something I'd done yet,
but I probably will at least try.

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

Actually control flow is not really something I'd put a lot of thought
into yet :-/

That said, I think most of what needs to go into a basic block structure
tends to be for optimization purposes (where optimization can even mean
"not having a terrible RA"), which is not something I've been hugely
concerned with as yet.