[PATCH vkd3d 5/8] vkd3d-shader/hlsl: Support addition for all numeric types in fold_constants().
Signed-off-by: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl_constant_ops.c | 45 ++++++++++++++++++++++++--- 1 file changed, 40 insertions(+), 5 deletions(-) diff --git a/libs/vkd3d-shader/hlsl_constant_ops.c b/libs/vkd3d-shader/hlsl_constant_ops.c index 3a778837..8279c58b 100644 --- a/libs/vkd3d-shader/hlsl_constant_ops.c +++ b/libs/vkd3d-shader/hlsl_constant_ops.c @@ -20,6 +20,43 @@ #include "hlsl.h" +static int constant_op2_add(struct hlsl_ctx *ctx, struct hlsl_ir_constant *tgt, + struct hlsl_ir_constant *src1, struct hlsl_ir_constant *src2) +{ + enum hlsl_base_type type = tgt->node.data_type->base_type; + uint32_t u1, u2; + + assert(type == src1->node.data_type->base_type); + assert(type == src2->node.data_type->base_type); + + for (int k = 0; k < 4; k++) + { + switch (type) + { + case HLSL_TYPE_FLOAT: + case HLSL_TYPE_HALF: + tgt->value[k].f = src1->value[k].f + src2->value[k].f; + break; + case HLSL_TYPE_DOUBLE: + tgt->value[k].d = src1->value[k].d + src2->value[k].d; + break; + case HLSL_TYPE_INT: + u1 = src1->value[k].i; + u2 = src2->value[k].i; + tgt->value[k].i = (int32_t)(u1 + u2); + break; + case HLSL_TYPE_UINT: + tgt->value[k].u = src1->value[k].u + src2->value[k].u; + break; + default: + FIXME("Fold \"%s\" for type %s.", debug_hlsl_expr_op(HLSL_OP2_ADD), + debug_hlsl_type(ctx, tgt->node.data_type)); + return 0; + } + } + return 1; +} + bool fold_constants(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, void *context) { struct hlsl_ir_constant *arg1, *arg2 = NULL, *res; @@ -49,6 +86,9 @@ bool fold_constants(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, void *cont switch (expr->op) { + case HLSL_OP2_ADD: + success = constant_op2_add(ctx, res, arg1, arg2); + break; default: goto fallback; } @@ -120,11 +160,6 @@ fallback: res->value[i].u = -arg1->value[i].u; break; - case HLSL_OP2_ADD: - for (i = 0; i < instr->data_type->dimx; ++i) - res->value[i].u = arg1->value[i].u + arg2->value[i].u; - break; - case HLSL_OP2_MUL: for (i = 0; i < instr->data_type->dimx; ++i) res->value[i].u = arg1->value[i].u * arg2->value[i].u; -- 2.25.1
Hi, On 06/01/22 18:39, Francisco Casas wrote:
+ case HLSL_TYPE_INT: + u1 = src1->value[k].i; + u2 = src2->value[k].i; + tgt->value[k].i = (int32_t)(u1 + u2); + break;
Maybe it makes sense to drop a comment here to make it clear why you're doing this? It might not be completely obvious to anybody. Something like /* Using unsigned to avoid undefined behavior on signed overflow. */. Same thing for the other patches. Giovanni.
On 1/6/22 11:39, Francisco Casas wrote:
Signed-off-by: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl_constant_ops.c | 45 ++++++++++++++++++++++++--- 1 file changed, 40 insertions(+), 5 deletions(-)
diff --git a/libs/vkd3d-shader/hlsl_constant_ops.c b/libs/vkd3d-shader/hlsl_constant_ops.c index 3a778837..8279c58b 100644 --- a/libs/vkd3d-shader/hlsl_constant_ops.c +++ b/libs/vkd3d-shader/hlsl_constant_ops.c @@ -20,6 +20,43 @@
#include "hlsl.h"
+static int constant_op2_add(struct hlsl_ctx *ctx, struct hlsl_ir_constant *tgt,
"dst" is a bit more usual than "tgt". Also, why does this function return int instead of bool?
+ struct hlsl_ir_constant *src1, struct hlsl_ir_constant *src2) +{ + enum hlsl_base_type type = tgt->node.data_type->base_type; + uint32_t u1, u2; + + assert(type == src1->node.data_type->base_type); + assert(type == src2->node.data_type->base_type); + + for (int k = 0; k < 4; k++) + { + switch (type) + { + case HLSL_TYPE_FLOAT: + case HLSL_TYPE_HALF: + tgt->value[k].f = src1->value[k].f + src2->value[k].f; + break; + case HLSL_TYPE_DOUBLE: + tgt->value[k].d = src1->value[k].d + src2->value[k].d; + break; + case HLSL_TYPE_INT: + u1 = src1->value[k].i; + u2 = src2->value[k].i; + tgt->value[k].i = (int32_t)(u1 + u2); + break;
It seems simpler just to fall through to the next case.
+ case HLSL_TYPE_UINT: + tgt->value[k].u = src1->value[k].u + src2->value[k].u; + break; + default: + FIXME("Fold \"%s\" for type %s.", debug_hlsl_expr_op(HLSL_OP2_ADD), + debug_hlsl_type(ctx, tgt->node.data_type)); + return 0; + } + } + return 1; +} +
January 17, 2022 8:51 PM, "Zebediah Figura (she/her)" <zfigura(a)codeweavers.com> wrote:
On 1/6/22 11:39, Francisco Casas wrote:
Signed-off-by: Francisco Casas <fcasas(a)codeweavers.com> --- libs/vkd3d-shader/hlsl_constant_ops.c | 45 ++++++++++++++++++++++++--- 1 file changed, 40 insertions(+), 5 deletions(-) diff --git a/libs/vkd3d-shader/hlsl_constant_ops.c b/libs/vkd3d-shader/hlsl_constant_ops.c index 3a778837..8279c58b 100644 --- a/libs/vkd3d-shader/hlsl_constant_ops.c +++ b/libs/vkd3d-shader/hlsl_constant_ops.c @@ -20,6 +20,43 @@
#include "hlsl.h" +static int constant_op2_add(struct hlsl_ctx *ctx, struct hlsl_ir_constant *tgt,
"dst" is a bit more usual than "tgt".
Ok, I will change the name.
Also, why does this function return int instead of bool?
Sorry, force of habit.
+ struct hlsl_ir_constant *src1, struct hlsl_ir_constant *src2) +{ + enum hlsl_base_type type = tgt->node.data_type->base_type; + uint32_t u1, u2; + + assert(type == src1->node.data_type->base_type); + assert(type == src2->node.data_type->base_type); + + for (int k = 0; k < 4; k++) + { + switch (type) + { + case HLSL_TYPE_FLOAT: + case HLSL_TYPE_HALF: + tgt->value[k].f = src1->value[k].f + src2->value[k].f; + break; + case HLSL_TYPE_DOUBLE: + tgt->value[k].d = src1->value[k].d + src2->value[k].d; + break; + case HLSL_TYPE_INT: + u1 = src1->value[k].i; + u2 = src2->value[k].i; + tgt->value[k].i = (int32_t)(u1 + u2); + break;
It seems simpler just to fall through to the next case.
That's clever, but maybe too clever! While it should work, I am inclined to leave it as it is, for readability and for having the peace of mind that it won't surprise us in the future. We would have to remember to keep hlsl_constant_value as a union and, for instance, I have the same concern as this person: https://stackoverflow.com/questions/11373203/accessing-inactive-union-member... and the answers don't seem conclusive.
+ case HLSL_TYPE_UINT: + tgt->value[k].u = src1->value[k].u + src2->value[k].u; + break; + default: + FIXME("Fold \"%s\" for type %s.", debug_hlsl_expr_op(HLSL_OP2_ADD), + debug_hlsl_type(ctx, tgt->node.data_type)); + return 0; + } + } + return 1; +} +
On 1/19/22 12:30, Francisco Casas wrote:
+ struct hlsl_ir_constant *src1, struct hlsl_ir_constant *src2) +{ + enum hlsl_base_type type = tgt->node.data_type->base_type; + uint32_t u1, u2; + + assert(type == src1->node.data_type->base_type); + assert(type == src2->node.data_type->base_type); + + for (int k = 0; k < 4; k++) + { + switch (type) + { + case HLSL_TYPE_FLOAT: + case HLSL_TYPE_HALF: + tgt->value[k].f = src1->value[k].f + src2->value[k].f; + break; + case HLSL_TYPE_DOUBLE: + tgt->value[k].d = src1->value[k].d + src2->value[k].d; + break; + case HLSL_TYPE_INT: + u1 = src1->value[k].i; + u2 = src2->value[k].i; + tgt->value[k].i = (int32_t)(u1 + u2); + break;
It seems simpler just to fall through to the next case.
That's clever, but maybe too clever! While it should work, I am inclined to leave it as it is, for readability and for having the peace of mind that it won't surprise us in the future.
We would have to remember to keep hlsl_constant_value as a union and, for instance, I have the same concern as this person: https://stackoverflow.com/questions/11373203/accessing-inactive-union-member... and the answers don't seem conclusive.
Type punning is allowed, and if int32_t can't be correctly reinterpreted as uint32_t we have bigger problems.
Hi, Il 19/01/22 19:30, Francisco Casas ha scritto:
That's clever, but maybe too clever! While it should work, I am inclined to leave it as it is, for readability and for having the peace of mind that it won't surprise us in the future.
We would have to remember to keep hlsl_constant_value as a union and, for instance, I have the same concern as this person: https://stackoverflow.com/questions/11373203/accessing-inactive-union-member... and the answers don't seem conclusive.
I agree with Zebediah, in our case it's ok to do that. More specifically, notice that that question was about C++, not about C, and often the two languages tend to differ in this kind of things. Answers are both about C and C++, but at least for C this is not undefined behavior (see for example the first part of https://stackoverflow.com/a/11996970/807307). It is still implementation-defined, but here and in many other places we assume that we use GCC (or a compatible thing), which defines it the way we like: https://gcc.gnu.org/onlinedocs/gcc-4.7.1/gcc/Structures-unions-enumerations-... Giovanni.
January 20, 2022 5:48 AM, "Giovanni Mascellani" <gmascellani(a)codeweavers.com> wrote:
Hi,
Il 19/01/22 19:30, Francisco Casas ha scritto:
That's clever, but maybe too clever! While it should work, I am inclined to leave it as it is, for readability and for having the peace of mind that it won't surprise us in the future. We would have to remember to keep hlsl_constant_value as a union and, for instance, I have the same concern as this person: https://stackoverflow.com/questions/11373203/accessing-inactive-union-member... and the answers don't seem conclusive.
I agree with Zebediah, in our case it's ok to do that.
More specifically, notice that that question was about C++, not about C, and often the two languages tend to differ in this kind of things. Answers are both about C and C++, but at least for C this is not undefined behavior (see for example the first part of https://stackoverflow.com/a/11996970/807307). It is still implementation-defined, but here and in many other places we assume that we use GCC (or a compatible thing), which defines it the way we like:
https://gcc.gnu.org/onlinedocs/gcc-4.7.1/gcc/Structures-unions-enumerations-... entation.html#Structures-unions-enumerations-and-bit_002dfields-implementation
Giovanni.
Roger that. In that case I will also do it for multiplication and negation. Francisco.
participants (3)
-
Francisco Casas -
Giovanni Mascellani -
Zebediah Figura (she/her)