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

[Giovanni Mascellani]

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.

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

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

>>           {
>> -            FIXME("Constructor argument with %u components.\n", width);
>> -            continue;
>> -        }
>> +            struct hlsl_ir_node *value;
>> -        if (!(arg = add_implicit_conversion(ctx, params->instrs, arg,
>> -                hlsl_get_vector_type(ctx, type->base_type, width), 
>> &arg->loc)))
>> -            continue;
>> +            if (!(value = load_index(ctx, params->instrs, arg, j, 
>> &loc)))
>> +                return NULL;
>> -        if (!(store = hlsl_new_store(ctx, var, NULL, arg,
>> -                ((1u << width) - 1) << writemask_offset, arg->loc)))
>> -            return NULL;
>> -        list_add_tail(params->instrs, &store->node.entry);
>> +            if (!(value = add_implicit_conversion(ctx, 
>> params->instrs, value, scal_type, &arg->loc)))
>> +                return NULL;
>> -        writemask_offset += width;
>> +            if (!store_index(ctx, params->instrs, var, value, idx, 
>> &loc))
>> +                return NULL;
>> +        }
>>       }
>>       if (!(load = hlsl_new_var_load(ctx, var, loc)))
> 
> This can be follow-up patches, but as long as you're implementing it 
> here, would you mind adding tests for constructors containing vectors 
> and matrices?

Ok.

Giovanni.