[PATCH vkd3d v7 4/6] vkd3d-shader/hlsl: Perform a copy propagation pass.

[Giovanni Mascellani]

Hi,

On 18/11/21 18:28, Matteo Bruni wrote:
>> +struct copy_propagation_value
>> +{
>> +    struct hlsl_ir_node *node;
>> +    unsigned int component;
>> +};
>> +
>> +struct copy_propagation_variable
>> +{
>> +    struct rb_entry entry;
>> +    struct hlsl_ir_var *var;
>> +    struct copy_propagation_value *values;
>> +};
> 
> I still haven't gotten warm to these names. What about
> copy_propagation_definition (or just copy_propagation_def)?

I don't understand why it's appropriate for this structure, but fine.

> If we want to go ahead with this dynamic / arbitrary variable size
> thing (which I still don't like, but I give up), at least let's avoid
> a separate allocation for the values, i.e.
> 
> struct copy_propagation_variable
> {
>      struct rb_entry entry;
>      struct hlsl_ir_var *var;
>      struct copy_propagation_value values[];
> };
> 
> variable = hlsl_alloc(ctx, offsetof(struct copy_propagation_variable,
> values[var->data_type->reg_size]);

I knew that was coming...

I don't like at all this pattern, but since you gave up on your side 
I'll do the same on mine.

>> +static struct hlsl_ir_node *copy_propagation_find_replacement(struct copy_propagation_variable *variable,
>> +        unsigned int offset, unsigned int count, unsigned int *swizzle)
> 
> I think I prefer my original suggestion of _get_ (or something else)
> rather than _find_; the point being that this function isn't really
> finding anything.

To me it gets something as much as it finds something. I wanted to avoid 
the word "get" because it seems overly generic, but it's true that 
"find" is not that much more specific. Ok.

> This and copy_propagation_transform_block() are a bit of a
> reimplementation of transform_ir(). It should be possible to extend
> transform_ir() to handle this new pass instead.
> 
> I'd introduce a struct like
> 
> struct transform_pass
> {
>      bool (*initialize)(struct hlsl_ctx *hlsl_ctx, void **ctx);
>      bool (*process_instruction)(void *ctx, struct hlsl_ir_node *, void *);
>      void (*destroy)(void *ctx);
> };
> 
> and update transform_ir() and the existing passes to make use of it. I
> imagine we could have some generic initialize() implementation simply
> doing *ctx = hlsl_ctx; and an empty destroy() implementation to be
> used when there is no particular context to carry around (like in the
> existing passes).

I agree, and that happened in my initial submission, with a slightly 
different interface. For each conditional block the callback must be 
called thrice (before the block, at the else and after the block) and 
for each loop block it must be called twice. It seemed to be a 
reasonable framework which might be useful for other passes in the future.

Zeb asked me to remove it until it is clear that there are other passes 
that are going to use it, so I custom coded everything.

Since it seems that in the meantime Zeb already managed to convince you 
to leave it like this, I won't touch it.

Giovanni.