Re: [PATCH vkd3d 4/6] vkd3d-shader/hlsl: Lower narrowing casts to swizzles.

[Zebediah Figura]

On 11/23/21 5:10 AM, Giovanni Mascellani wrote:
> Signed-off-by: Giovanni Mascellani
>
> On 23/11/21 02:45, Zebediah Figura wrote:
> > +static bool lower_narrowing_casts(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, void *context)
> > +{
> > - const struct hlsl_type *src_type, *dst_type;
> > - struct hlsl_ir_expr *cast;
> > -
> > - if (instr->type != HLSL_IR_EXPR)
> > -
> >    return false;
> > - cast = hlsl_ir_expr(instr);
> > - src_type = cast->operands[0].node->data_type;
> > - dst_type = cast->node.data_type;
> > -
> > - if (cast->op == HLSL_OP1_CAST
> > -
> >        && src_type->type <= HLSL_CLASS_VECTOR && dst_type->type <= HLSL_CLASS_VECTOR
> > -
> >        && dst_type->dimx < src_type->dimx)
> > - {
> > -
> >    struct hlsl_ir_swizzle *swizzle;
> > -
> > -
> >    if (!(swizzle = hlsl_new_swizzle(ctx, HLSL_SWIZZLE(X, Y, Z, W), dst_type->dimx, &cast->node, &cast->node.loc)))
> > -
> >        return false;
> > -
> >    list_add_after(&cast->node.entry, &swizzle->node.entry);
> > -
> > -
> >    cast->node.data_type = hlsl_get_vector_type(ctx, dst_type->base_type, src_type->dimx);
> > -
> >    replace_node(&cast->node, &swizzle->node);
> > -
> >    hlsl_src_remove(&swizzle->val);
> > -
> >    hlsl_src_from_node(&swizzle->val, &cast->node);
> > -
> >    return true;
> > - }
> > -
> > - return false;
> >
> > +}
>
> The same comments of 3/6 apply here.
>
> While reviewing this patch it occurred to me that both this and 3/6 can
> change the type of a node, by turning a scalar into a vector of
> dimension 1. I guess that by this point the code is sufficiently
> low-level that we don't care any more, but since this a bit vague, I'd
> propose to identify the point where all scalars in the IR can be
> converted to vectors of dimension 1, and from that point on scalars are
> not used any more. What do you think of that? (I like strongly types
> data...)

I don't know that it's worth the trouble. Any given operation should be 
able to support vec1's as well as scalars. We have a helper for that, 
after all.
> > @@ -1706,6 +1737,7 @@ int hlsl_emit_dxbc(struct hlsl_ctx *ctx, struct hlsl_ir_function_decl *entry_fun
> >            progress |= transform_ir(ctx, split_struct_copies, body, NULL);
> >        }
> >        while (progress);
> > - transform_ir(ctx, lower_narrowing_casts, body, NULL);
> >   do
> >   {
> >       progress = transform_ir(ctx, fold_constants, body, NULL);
>
> Is there a specific reason for how you are ordering this pass (and
> 3/6?). Not that I have any problem with it, it is just that 4/6 seems
> similar to 3/6, but you put them in two different places.
>
> For the record, I have another pass in my development patches that
> generalizes both 3/6 and 4/6 and also works with matrices. It will take
> some time to have it accepted, if it will ever be, so it should not be a
> reason to delay these two patches.

I don't think there's any particular reason, no.