[PATCH vkd3d] vkd3d-shader/hlsl: Support casts between all numeric types on constant folding.
Zebediah Figura (she/her)
zfigura at codeweavers.com
Thu Dec 30 12:06:18 CST 2021
On 12/29/21 08:51, Francisco Casas wrote:
> Signed-off-by: Francisco Casas <fcasas at codeweavers.com>
> ---
>
> This is a proposal to handle all numeric constants in the same way.
>
> Signed-off-by: Francisco Casas <fcasas at codeweavers.com>
> ---
> Makefile.am | 1 -
> libs/vkd3d-shader/hlsl.c | 54 ++++++++++++++++++++++++++++++++
> libs/vkd3d-shader/hlsl.h | 3 ++
> libs/vkd3d-shader/hlsl_codegen.c | 48 ++++++++++------------------
> 4 files changed, 73 insertions(+), 33 deletions(-)
>
The approach seems sensible to me. I don't see a need to put the
function in hlsl.c, though.
> diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c
> index d2ea4c34..a44b638e 100644
> --- a/libs/vkd3d-shader/hlsl.c
> +++ b/libs/vkd3d-shader/hlsl.c
> @@ -1663,6 +1663,60 @@ unsigned int hlsl_combine_swizzles(unsigned int first, unsigned int second, unsi
> return ret;
> }
>
> +void hlsl_cast_constant_value(struct hlsl_ir_constant *con, enum hlsl_base_type current,
> + enum hlsl_base_type target){
> + uint32_t u; int32_t i; float f; double d;
Please try to avoid putting multiple statements or declarations on one line.
> +
> + assert(con);
This assert seems unnecessary.
> + for (int k = 0; k < 4; k++)
> + {
> + switch (current)
> + {
> + case HLSL_TYPE_FLOAT:
> + case HLSL_TYPE_HALF:
> + u = con->value[k].f; i = con->value[k].f; f = con->value[k].f; d = con->value[k].f;
> + break;
> + case HLSL_TYPE_DOUBLE:
> + u = con->value[k].d; i = con->value[k].d; f = con->value[k].d; d = con->value[k].d;
> + break;
> + case HLSL_TYPE_INT:
> + u = con->value[k].i; i = con->value[k].i; f = con->value[k].i; d = con->value[k].i;
> + break;
> + case HLSL_TYPE_UINT:
> + u = con->value[k].u; i = con->value[k].u; f = con->value[k].u; d = con->value[k].u;
> + break;
> + case HLSL_TYPE_BOOL:
> + u = !!con->value[k].u; i = !!con->value[k].u; f = !!con->value[k].u; d = !!con->value[k].u;
> + break;
> + default:
> + assert(0);
> + break;
> + }
> + switch (target)
> + {
> + case HLSL_TYPE_FLOAT:
> + case HLSL_TYPE_HALF:
> + con->value[k].f = f;
> + break;
> + case HLSL_TYPE_DOUBLE:
> + con->value[k].d = d;
> + break;
> + case HLSL_TYPE_INT:
> + con->value[k].i = i;
> + break;
> + case HLSL_TYPE_UINT:
> + con->value[k].u = u;
> + break;
> + case HLSL_TYPE_BOOL:
> + con->value[k].u = (!!u) * 0xffffffff;
> + break;
> + default:
> + assert(0);
> + break;
> + }
> + }
> +}
> +
> static const struct hlsl_profile_info *get_target_info(const char *target)
> {
> unsigned int i;
> diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h
> index 57acf3a0..a07ccf17 100644
> --- a/libs/vkd3d-shader/hlsl.h
> +++ b/libs/vkd3d-shader/hlsl.h
> @@ -755,6 +755,9 @@ unsigned int hlsl_combine_writemasks(unsigned int first, unsigned int second);
> unsigned int hlsl_map_swizzle(unsigned int swizzle, unsigned int writemask);
> unsigned int hlsl_swizzle_from_writemask(unsigned int writemask);
>
> +void hlsl_cast_constant_value(struct hlsl_ir_constant *con, enum hlsl_base_type current,
> + enum hlsl_base_type target);
> +
> bool hlsl_offset_from_deref(const struct hlsl_deref *deref, unsigned int *offset);
> unsigned int hlsl_offset_from_deref_safe(struct hlsl_ctx *ctx, const struct hlsl_deref *deref);
> struct hlsl_reg hlsl_reg_from_deref(struct hlsl_ctx *ctx, const struct hlsl_deref *deref,
> diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c
> index 75716bdf..ef627f2b 100644
> --- a/libs/vkd3d-shader/hlsl_codegen.c
> +++ b/libs/vkd3d-shader/hlsl_codegen.c
> @@ -656,7 +656,7 @@ static bool fold_constants(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, voi
> {
> struct hlsl_ir_constant *arg1, *arg2 = NULL, *res;
> struct hlsl_ir_expr *expr;
> - unsigned int i, dimx;
> + unsigned int i;
>
> if (instr->type != HLSL_IR_EXPR)
> return false;
> @@ -670,48 +670,32 @@ static bool fold_constants(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, voi
> arg1 = hlsl_ir_constant(expr->operands[0].node);
> if (expr->operands[1].node)
> arg2 = hlsl_ir_constant(expr->operands[1].node);
> - dimx = instr->data_type->dimx;
>
> if (!(res = hlsl_alloc(ctx, sizeof(*res))))
> return false;
> init_node(&res->node, HLSL_IR_CONSTANT, instr->data_type, instr->loc);
>
> + if (expr->op == HLSL_OP1_CAST && instr->data_type->base_type <= HLSL_TYPE_LAST_SCALAR)
Frankly we may want to consider swapping the "switch" block order then.
Especially considering that we might want to add other operations that
aren't typed.
> + {
> + if (instr->data_type->dimx != arg1->node.data_type->dimx
> + || instr->data_type->dimy != arg1->node.data_type->dimy)
> + {
> + WARN("Cast from %s to %s.\n", debug_hlsl_type(ctx, arg1->node.data_type),
> + debug_hlsl_type(ctx, instr->data_type));
Why remove the "return false" from this? Also, why change it from a FIXME?
> + }
> + memcpy(res->value, arg1->value, sizeof(res->value));
What's the point of copying to the value if we're just going to
overwrite it?
> + hlsl_cast_constant_value(res, arg1->node.data_type->base_type, instr->data_type->base_type);
> + list_add_before(&expr->node.entry, &res->node.entry);
> + replace_node(&expr->node, &res->node);
> + return res;
fold_constants() returns bool. If you're not compiling with -Werror
already I'd recommend it.
> + }
> +
More information about the wine-devel
mailing list