[PATCH vkd3d 08/12] vkd3d-shader/hlsl: Parse matrix constructors.

[Zebediah Figura]

On 4/21/22 04:53, Giovanni Mascellani wrote:
> Hi,
> 
> Il 20/04/22 20:47, Zebediah Figura ha scritto:
>>> +static struct hlsl_ir_node *compute_offset_from_index(struct 
>>> hlsl_ctx *ctx, struct list *instrs,
>>> +        struct hlsl_type *type, unsigned int idx, const struct 
>>> vkd3d_shader_location *loc)
>>> +{
>>> +    assert(type->type <= HLSL_CLASS_LAST_NUMERIC);
>>> +    assert(idx < type->dimx * type->dimy);
>>> +
>>> +    if (type->type == HLSL_CLASS_SCALAR || type->type == 
>>> HLSL_CLASS_VECTOR)
>>> +    {
>>> +        struct hlsl_ir_constant *c;
>>> +
>>> +        if (!(c = hlsl_new_uint_constant(ctx, idx, loc)))
>>> +            return NULL;
>>> +        list_add_tail(instrs, &c->node.entry);
>>> +
>>> +        return &c->node;
>>> +    }
>>> +    else
>>> +    {
>>> +        struct hlsl_ir_constant *x, *y;
>>> +
>>> +        if (!(x = hlsl_new_uint_constant(ctx, idx % type->dimx, loc)))
>>> +            return NULL;
>>> +        list_add_tail(instrs, &x->node.entry);
>>> +
>>> +        if (!(y = hlsl_new_uint_constant(ctx, idx / type->dimx, loc)))
>>> +            return NULL;
>>> +        list_add_tail(instrs, &y->node.entry);
>>> +
>>> +        return compute_matrix_offset(ctx, instrs, type, &x->node, 
>>> &y->node, loc);
>>> +    }
>>> +}
>>
>> I guess, but have you considered just returning an unsigned int from 
>> this function? It seems like potentially less work [and wouldn't 
>> invalidate my comment on compute_matrix_offset()...]
> 
> That would require to duplicate the logic behind compute_matrix_offset() 
> (once for computing in the compiler, one for computing in the shader). 
> Granted, it's not a particularly complicated logic, but still I'd prefer 
> to keep one copy of it.

I dunno, I see the logic as not particularly complex...

I'm also slightly concerned that we're generating terrible code too 
zealously. Simplicity of implementation is valuable, but it means that 
optimization passes have more busywork to do, and this is one of those 
cases where I'm not sure it's actually worth it...

> 
>> If not, I'd prefer to name this function something that makes it 
>> obvious that it's modifying the instruction stream (i.e. it should 
>> begin with add_*, probably). That goes for load_index() and 
>> store_index() as well.
> 
> To me that's sort of obvious, given that we're passing the instruction 
> list as a parameter, but I can do that. I suppose that would apply to 
> compute_matrix_offset() too, then.

I find naming to be important enough that it's worth the extra clarity 
even if the argument types are unambiguous.

> 
>>>           width = hlsl_type_component_count(arg->data_type);
>>> -        if (width > 4)
>>> +        for (j = 0; j < width; ++j, ++idx)
>>
>> I know this is a common construction, but it always throws me off. I 
>> can live with it, but I'd prefer to put the ++idx in the store_index() 
>> call.
> 
> OTOH, I don't like very much when operators mutating data is buried 
> inside a larger formula, except when it happens at very idiomatic 
> positions, like "x[i++] = ..." or "*ptr++ = ...", or of course in the 
> third slot of a "for" statement. "x = y = z" is already something that 
> hurts legibility a lot, IMHO.
> 
> But I can live with your suggestion, so I'll do it.

Sure, it's fine on a separate line too.

I guess not being used to multiple increments is a personal thing for 
me, so I probably should just drop this comment anyway.