Re: [PATCH vkd3d 08/11] vkd3d-shader/hlsl: Fold constant division.

[Matteo Bruni]

On Thu, Apr 14, 2022 at 12:53 PM Giovanni Mascellani
 wrote:
> This commit includes work by Francisco Casas.
> Signed-off-by: Giovanni Mascellani
>
> libs/vkd3d-shader/hlsl_constant_ops.c    | 67 ++++++++++++++++++++++++
>  libs/vkd3d-shader/vkd3d_shader_private.h |  2 +
>  tests/arithmetic-float.shader_test       | 16 ++++++
>  tests/arithmetic-int.shader_test         | 11 +++-
>  tests/arithmetic-uint.shader_test        |  9 ++++
>  5 files changed, 104 insertions(+), 1 deletion(-)
>
> diff --git a/libs/vkd3d-shader/hlsl_constant_ops.c b/libs/vkd3d-shader/hlsl_constant_ops.c
> index 5cac4bde..9a8bc6b0 100644
> --- a/libs/vkd3d-shader/hlsl_constant_ops.c
> +++ b/libs/vkd3d-shader/hlsl_constant_ops.c
> @@ -255,6 +255,69 @@ static bool fold_nequal(struct hlsl_ctx *ctx, struct hlsl_ir_constant *dst,
>      return true;
>  }
>
> +static bool fold_div(struct hlsl_ctx *ctx, struct hlsl_ir_constant *dst,
> -
>    struct hlsl_ir_constant *src1, struct hlsl_ir_constant *src2)
>
> +{
> - enum hlsl_base_type type = dst->node.data_type->base_type;
> - unsigned int k;
> -
> - assert(type == src1->node.data_type->base_type);
> - assert(type == src2->node.data_type->base_type);
> -
> - for (k = 0; k < dst->node.data_type->dimx; ++k)
> - {
> -
>    switch (type)
> -
>    {
> -
>        case HLSL_TYPE_FLOAT:
> -
>        case HLSL_TYPE_HALF:
> -
>            if (src2->value[k].f == 0)
> -
>            {
> -
>                hlsl_warning(ctx, &dst->node.loc, VKD3D_SHADER_WARNING_HLSL_DIVISION_BY_ZERO,
> -
>                        "Floating point division by zero");
> -
>            }
> -
>            dst->value[k].f = src1->value[k].f / src2->value[k].f;
> -
>            break;

This is in fact an error in SM1 (and the reason for the [require] in
the corresponding test, I guess). Do we have a patch for this lined up
already? Or did we decide to diverge from native in this particular
situation? In the latter case we probably want a comment though. A
comment next to the [require] would also be nice either way.
> -
> -
>        case HLSL_TYPE_DOUBLE:
> -
>            if (src2->value[k].d == 0)
> -
>            {
> -
>                hlsl_warning(ctx, &dst->node.loc, VKD3D_SHADER_WARNING_HLSL_DIVISION_BY_ZERO,
> -
>                        "Floating point division by zero");
> -
>            }
> -
>            dst->value[k].d = src1->value[k].d / src2->value[k].d;
> -
>            break;
> -
> -
>        case HLSL_TYPE_INT:
> -
>            if (src2->value[k].i == 0)
> -
>            {
> -
>                hlsl_error(ctx, &dst->node.loc, VKD3D_SHADER_ERROR_HLSL_DIVISION_BY_ZERO,
> -
>                        "Division by zero.");
> -
>                return false;
> -
>            }
> -
>            if (src1->value[k].i == INT_MIN && src2->value[k].i == -1)
> -
>                dst->value[k].i = INT_MIN;
> -
>            else
> -
>                dst->value[k].i = src1->value[k].i / src2->value[k].i;
> -
>            break;

It would be nice to have a test for that INT_MIN / -1 special case.