[PATCH vkd3d] vkd3d-shader/hlsl: Support casts between all numeric types on constant folding.
Francisco Casas
fcasas at codeweavers.com
Thu Dec 30 15:55:05 CST 2021
December 30, 2021 3:06 PM, "Zebediah Figura (she/her)" <zfigura at codeweavers.com> wrote:
> 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.
>
Okay, I am moving it to hlsl_codegen.c then, before fold_constants().
>> 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.
>
Okay, I felt that this function was a special case, but I will follow these standards more
rigorously from now on.
>> +
>> + assert(con);
>
> This assert seems unnecessary.
>
Ok
>> + 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.
>
I agree. And if we do that, maybe we can do functions like these:
> /* This macro is just to illustrate the similarity among the functions */
> #define CONSTANT_OP2_FUNCTION(function_name,operator) \
> void function_name(struct hlsl_ir_constant *res, struct hlsl_ir_constant *arg1, \
> struct hlsl_ir_constant *arg2, enum hlsl_base_type type) \
> { \
> for(int k=0; k<4; k++) \
> { \
> switch (type) \
> { \
> case HLSL_TYPE_FLOAT: \
> case HLSL_TYPE_HALF: \
> res->value[k].f = arg1->value[k].f operator arg2->value[k].f; \
> break; \
> case HLSL_TYPE_DOUBLE: \
> res->value[k].d = arg1->value[k].d operator arg2->value[k].d; \
> break; \
> case HLSL_TYPE_INT: \
> res->value[k].i = arg1->value[k].i operator arg2->value[k].i; \
> break; \
> case HLSL_TYPE_UINT: \
> res->value[k].u = arg1->value[k].u operator arg2->value[k].u; \
> break; \
> case HLSL_TYPE_BOOL: \
> res->value[k].u = !!((!!(arg1->value[k].u)) operator (!!(arg2->value[k].u))) * 0xffffffff; \
> break; \
> default: \
> assert(0); \
> break; \
> } \
> } \
> }
>
> CONSTANT_OP2_FUNCTION(constant_value_sum,+)
> CONSTANT_OP2_FUNCTION(constant_value_sub,-)
> CONSTANT_OP2_FUNCTION(constant_value_mult,+)
> CONSTANT_OP2_FUNCTION(constant_value_neg,* (-1) + 0 *) /* horrid? */
> CONSTANT_OP2_FUNCTION(constant_value_div,/) /* horrid? */
And call one of these on each case of the switch.
As using macros this way is too ugly, I think we should consider creating
a new "constant_ops.c" file to define all these boilerplate functions for
constant values. They may get even larger if we support matrices.
>> + {
>> + 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?
>
Casting to a vector type with a smaller dimension works in the native compiler,
albeit with a warning. Since hlsl_cast_constant_value() copies all 4 values even
if they are not used, these cases should be covered now.
However, yes, casting to a type with a larger dimension should result in an error,
unless it is from a scalar.
I recognize I didn't think it too much, I will handle these cases better.
>> + }
>> + 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() expects the original value to be in the same constant
on which the result is written, so it has to be copied on the new node first.
The alternative would be receiving both a source and target hlsl_ir_constant...
maybe it is better that way.
>> + 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.
>
Sorry, I missed that because I initially wrote this on another branch,
thanks for the tip.
>> + }
>> +
More information about the wine-devel
mailing list