From: "Francisco Casas" <fcasas@codeweavers.com>
Subject: Re: [PATCH vkd3d 5/8] vkd3d-shader/hlsl: Support addition for all numeric types in fold_constants().
Message-Id: <cbd16fd310c38ebafddb7f4a49d7cfda@codeweavers.com>
Date: Wed, 19 Jan 2022 18:30:18 +0000
In-Reply-To: <84c45e1a-9268-11e7-9503-5bff51cd32b2@codeweavers.com>
References: <84c45e1a-9268-11e7-9503-5bff51cd32b2@codeweavers.com> <20220106173949.82958-5-fcasas@codeweavers.com>

January 17, 2022 8:51 PM, "Zebediah Figura (she/her)" <zfigura@codeweavers.com> wrote:

> On 1/6/22 11:39, Francisco Casas wrote:
> 
>> Signed-off-by: Francisco Casas <fcasas@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-undefined-behavior
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;
>> +}
>> +