[PATCH vkd3d] vkd3d-shader/hlsl: Support casts between all numeric types on constant folding.
Signed-off-by: Francisco Casas <fcasas(a)codeweavers.com> --- This is a proposal to handle all numeric constants in the same way. Signed-off-by: Francisco Casas <fcasas(a)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(-) diff --git a/Makefile.am b/Makefile.am index 20fee06d..39003dfa 100644 --- a/Makefile.am +++ b/Makefile.am @@ -304,7 +304,6 @@ XFAIL_TESTS = \ tests/hlsl-storage-qualifiers.shader_test \ tests/hlsl-vector-indexing.shader_test \ tests/hlsl-vector-indexing-uniform.shader_test \ - tests/max.shader_test \ tests/sampler.shader_test \ tests/texture-load.shader_test \ tests/texture-load-typed.shader_test \ 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; + + assert(con); + 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) + { + 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)); + } + memcpy(res->value, arg1->value, sizeof(res->value)); + 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; + } + switch (instr->data_type->base_type) { case HLSL_TYPE_FLOAT: { switch (expr->op) { - case HLSL_OP1_CAST: - if (instr->data_type->dimx != arg1->node.data_type->dimx - || instr->data_type->dimy != arg1->node.data_type->dimy) - { - FIXME("Cast from %s to %s.\n", debug_hlsl_type(ctx, arg1->node.data_type), - debug_hlsl_type(ctx, instr->data_type)); - vkd3d_free(res); - return false; - } - - switch (arg1->node.data_type->base_type) - { - case HLSL_TYPE_INT: - for (i = 0; i < dimx; ++i) - res->value[i].f = arg1->value[i].i; - break; - - case HLSL_TYPE_UINT: - for (i = 0; i < dimx; ++i) - res->value[i].f = arg1->value[i].u; - break; - - default: - FIXME("Cast from %s to %s.\n", debug_hlsl_type(ctx, arg1->node.data_type), - debug_hlsl_type(ctx, instr->data_type)); - vkd3d_free(res); - return false; - } - break; - default: FIXME("Fold float op %#x.\n", expr->op); vkd3d_free(res); -- 2.25.1
On 12/29/21 08:51, Francisco Casas wrote:
Signed-off-by: Francisco Casas <fcasas(a)codeweavers.com> ---
This is a proposal to handle all numeric constants in the same way.
Signed-off-by: Francisco Casas <fcasas(a)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.
+ } +
On 12/30/21 12:06, Zebediah Figura (she/her) wrote:
@@ -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.
FWIW, this should be a separate patch, and can be deferred until later anyway.
December 30, 2021 3:06 PM, "Zebediah Figura (she/her)" <zfigura(a)codeweavers.com> wrote:
> On 12/29/21 08:51, Francisco Casas wrote:
>
>> Signed-off-by: Francisco Casas <fcasas(a)codeweavers.com>
>> ---
>> This is a proposal to handle all numeric constants in the same way.
>> Signed-off-by: Francisco Casas <fcasas(a)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.
>> + }
>> +
On 12/30/21 15:55, Francisco Casas wrote:
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.
Maybe. I'd prefer to avoid preprocessor macros, though, and defer any sort of code generation until it becomes necessary.
+ { + 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.
Sure, but we should handle that elsewhere. In fact we already have lower_narrowing_casts() for this.
+ } + 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.
Eh, indeed, I skimmed and misread it. I'm inclined to specify source and target separately, yes.
Hi. On 30/12/21 22:55, Francisco Casas wrote:
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? */
If you ask me, the neg thing is pretty horrid indeed, I'd avoid it. Also, I'm pretty sure that multiplying a float by zero is not guaranteed to give you zero (infinity * zero = NaN, I think, and I don't know about NaN * zero), so depending on how you use it it might give wrong results. If you want to go down that route, maybe it's better to add a corresponding CONSTANT_OP1_FUNCTION. Also for the OP2 variants, notice that C doesn't define the results for all the possible arguments given to these operations. For example division by zero is undefined (and often results in a crash, in particular I think it does on the architectures we target) and signed overflow is undefined too (things like INT_MAX + INT_MAX, INT_MIN * -1, etc). It seems that we don't use -fwrapv in vkd3d (differently from Wine), which means that the compiler won't scramble to fix things up for us. Giovanni.
On 1/4/22 08:17, Giovanni Mascellani wrote:
Hi.
On 30/12/21 22:55, Francisco Casas wrote:
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? */
If you ask me, the neg thing is pretty horrid indeed, I'd avoid it. Also, I'm pretty sure that multiplying a float by zero is not guaranteed to give you zero (infinity * zero = NaN, I think, and I don't know about NaN * zero), so depending on how you use it it might give wrong results. If you want to go down that route, maybe it's better to add a corresponding CONSTANT_OP1_FUNCTION.
Also for the OP2 variants, notice that C doesn't define the results for all the possible arguments given to these operations. For example division by zero is undefined (and often results in a crash, in particular I think it does on the architectures we target) and signed overflow is undefined too (things like INT_MAX + INT_MAX, INT_MIN * -1, etc). It seems that we don't use -fwrapv in vkd3d (differently from Wine), which means that the compiler won't scramble to fix things up for us.
Where are you getting that Wine uses -fwrapv? I don't see any mention of that in the source.
Hi, On 29/12/21 15:51, Francisco Casas wrote:
switch (expr->op) { - case HLSL_OP1_CAST: - if (instr->data_type->dimx != arg1->node.data_type->dimx - || instr->data_type->dimy != arg1->node.data_type->dimy) - { - FIXME("Cast from %s to %s.\n", debug_hlsl_type(ctx, arg1->node.data_type), - debug_hlsl_type(ctx, instr->data_type)); - vkd3d_free(res); - return false; - } - - switch (arg1->node.data_type->base_type) - { - case HLSL_TYPE_INT: - for (i = 0; i < dimx; ++i) - res->value[i].f = arg1->value[i].i; - break; - - case HLSL_TYPE_UINT: - for (i = 0; i < dimx; ++i) - res->value[i].f = arg1->value[i].u; - break; - - default: - FIXME("Cast from %s to %s.\n", debug_hlsl_type(ctx, arg1->node.data_type), - debug_hlsl_type(ctx, instr->data_type)); - vkd3d_free(res); - return false; - } - break; - default: FIXME("Fold float op %#x.\n", expr->op); vkd3d_free(res);
Given that you are removing all the logic from the case HLSL_TYPE_FLOAT, you might as well remove the case block itself, I guess. Though I don't have strong opinions on that. Eventually we'll put some new code in it anyway. Giovanni.
participants (3)
-
Francisco Casas -
Giovanni Mascellani -
Zebediah Figura (she/her)