[PATCH vkd3d] vkd3d-shader/hlsl: Support casts between all numeric types on constant folding.
Signed-off-by: Francisco Casas fcasas@codeweavers.com ---
This is a proposal to handle all numeric constants in the same way.
Signed-off-by: Francisco Casas fcasas@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);
On 12/29/21 08:51, Francisco Casas wrote:
The approach seems sensible to me. I don't see a need to put the function in hlsl.c, though.
Please try to avoid putting multiple statements or declarations on one line.
This assert seems unnecessary.
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.
Why remove the "return false" from this? Also, why change it from a FIXME?
What's the point of copying to the value if we're just going to overwrite it?
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:
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@codeweavers.com wrote:
Okay, I am moving it to hlsl_codegen.c then, before fold_constants().
Okay, I felt that this function was a special case, but I will follow these standards more rigorously from now on.
Ok
I agree. And if we do that, maybe we can do functions like these:
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.
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.
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.
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:
Maybe. I'd prefer to avoid preprocessor macros, though, and defer any sort of code generation until it becomes necessary.
Sure, but we should handle that elsewhere. In fact we already have lower_narrowing_casts() for this.
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:
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:
Where are you getting that Wine uses -fwrapv? I don't see any mention of that in the source.
Hi,
Il 18/01/22 00:47, Zebediah Figura (she/her) ha scritto:
Where are you getting that Wine uses -fwrapv? I don't see any mention of that in the source.
My bad, I got confused. It uses -fno-strict-aliasing, but not -fwrapv.
Giovanni.
Hi,
On 29/12/21 15:51, Francisco Casas wrote:
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.
-
Francisco Casas -
Giovanni Mascellani -
Zebediah Figura (she/her)