From: "Zebediah Figura (she/her)" Subject: Re: [PATCH vkd3d] vkd3d-shader/hlsl: Support casts between all numeric types on constant folding. Message-Id: <658b9e98-ef2e-d928-8bb0-5da1664b92fc@codeweavers.com> Date: Thu, 30 Dec 2021 12:06:18 -0600 In-Reply-To: <20211229145154.123397-1-fcasas@codeweavers.com> References: <20211229145154.123397-1-fcasas@codeweavers.com> On 12/29/21 08:51, Francisco Casas wrote: > Signed-off-by: Francisco Casas > --- > > This is a proposal to handle all numeric constants in the same way. > > Signed-off-by: Francisco Casas > --- > 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. > + } > +