[PATCH 0/1] MR54: vkd3d-shader/hlsl: Initialize static variables to 0 by default.

[Francisco Casas (＠fcasas)]

We are currently not initializing static values to zero by default.

Consider the following shader:
```hlsl
static float4 va;

float4 main() : sv_target
{
  return va;
}
```
we get the following output:
```
ps_5_0
dcl_output o0.xyzw
dcl_temps 2
mov r0.xyzw, r1.xyzw
mov o0.xyzw, r0.xyzw
ret
```
where r1.xyzw is not initialized.

This patch solves this by assigning the static variable the value of an
uint 0, and thus, relying on complex broadcasts.

This seems to be the behaviour of the the native compiler, since it retrieves
the following error on a shader that lacks an initializer on a data type with
object components:

```
error X3017: cannot convert from 'uint' to 'struct <unnamed>'
```

[PATCH 1/1] vkd3d-shader/hlsl: Initialize static variables to 0 by default.

[Francisco Casas]

From: Francisco Casas

We are currently not initializing static values to zero by default.

Consider the following shader:
```hlsl
static float4 va;

float4 main() : sv_target
{
  return va;
}
```
we get the following output:
```
ps_5_0
dcl_output o0.xyzw
dcl_temps 2
mov r0.xyzw, r1.xyzw
mov o0.xyzw, r0.xyzw
ret
```
where r1.xyzw is not initialized.

This patch solves this by assigning the static variable the value of an
uint 0, and thus, relying on complex broadcasts.

This seems to be the behaviour of the the native compiler, since it retrieves
the following error on a shader that lacks an initializer on a data type with
object components:

```
error X3017: cannot convert from 'uint' to 'struct <unnamed>'
```
---
 libs/vkd3d-shader/hlsl.y       | 35 ++++++++++++++++++++++++++++++++++
 tests/hlsl-invalid.shader_test |  8 ++++++++
 2 files changed, 43 insertions(+)

diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
index eedc85bd..a10f7308 100644
--- a/libs/vkd3d-shader/hlsl.y
+++ b/libs/vkd3d-shader/hlsl.y
@@ -2167,6 +2167,41 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t
             vkd3d_free(v->initializer.args);
             vkd3d_free(v->initializer.instrs);
         }
+        else if (var->modifiers & HLSL_STORAGE_STATIC)
+        {
+            struct hlsl_ir_constant *zero;
+            struct hlsl_ir_load *load;
+
+            /* Initialize statics to zero by default. */
+
+            if (type_has_object_components(var->data_type, false))
+            {
+                hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_MISSING_INITIALIZER,
+                        "Static variable cannot be initialized to 0 if it has object components.");
+                vkd3d_free(v);
+                continue;
+            }
+
+            if (!(zero = hlsl_new_uint_constant(ctx, 0, &var->loc)))
+            {
+                vkd3d_free(v);
+                continue;
+            }
+            list_add_tail(&ctx->static_initializers, &zero->node.entry);
+
+            if (!(load = hlsl_new_var_load(ctx, var, var->loc)))
+            {
+                vkd3d_free(v);
+                continue;
+            }
+            list_add_tail(&ctx->static_initializers, &load->node.entry);
+
+            if (!add_assignment(ctx, &ctx->static_initializers, &load->node, ASSIGN_OP_ASSIGN, &zero->node))
+            {
+                vkd3d_free(v);
+                continue;
+            }
+        }
         vkd3d_free(v);
     }
     vkd3d_free(var_list);
diff --git a/tests/hlsl-invalid.shader_test b/tests/hlsl-invalid.shader_test
index 1b39d7f0..3e18961b 100644
--- a/tests/hlsl-invalid.shader_test
+++ b/tests/hlsl-invalid.shader_test
@@ -264,3 +264,11 @@ float4 main() : sv_target
 {
     return 1.0;
 }
+
+[pixel shader fail]
+static uint i;
+float4 main() : sv_target { return 1/i; }
+
+[pixel shader fail]
+static Texture2D tex;
+float4 main() : sv_target { return 0; }

[Giovanni Mascellani (＠giomasce)]

Giovanni Mascellani (@giomasce) commented about tests/hlsl-invalid.shader_test:
> {
>      return 1.0;
>  }
> -
>
> +[pixel shader fail]
> +static uint i;
> +float4 main() : sv_target { return 1/i; }
> -
>
> +[pixel shader fail]
> +static Texture2D tex;
> +float4 main() : sv_target { return 0; }

This seems to compile here, at least with profiles `ps_4_0`, `ps_5_0` and `ps_5_1`: https://shader-playground.timjones.io/4e83a24178541aea41b6cef402cd564f.

[Giovanni Mascellani (＠giomasce)]

Giovanni Mascellani (@giomasce) commented about tests/hlsl-invalid.shader_test:
> {
>      return 1.0;
>  }
> -
>
> +[pixel shader fail]
> +static uint i;
> +float4 main() : sv_target { return 1/i; }
> -
>
> +[pixel shader fail]
> +static Texture2D tex;
> +float4 main() : sv_target { return 0; }

I think they should be formatted properly, even if they are really short.

[Giovanni Mascellani (＠giomasce)]

Giovanni Mascellani (@giomasce) commented about tests/hlsl-invalid.shader_test:
> {
>      return 1.0;
>  }
> -
>
> +[pixel shader fail]
> +static uint i;
> +float4 main() : sv_target { return 1/i; }

It's good to test for this failure, but we should probably also have a succeeding test for the usual case (having a static numeric variable, returning it and showing it is zero).

[Giovanni Mascellani (＠giomasce)]

Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/hlsl.y:
>          vkd3d_free(v->initializer.args);
>          vkd3d_free(v->initializer.instrs);
>      }
> -
>    else if (var->modifiers & HLSL_STORAGE_STATIC)
> -
>    {
> -
>        struct hlsl_ir_constant *zero;
> -
>        struct hlsl_ir_load *load;
> -
> -
>        /* Initialize statics to zero by default. */
> -
> -
>        if (type_has_object_components(var->data_type, false))
> -
>        {
> -
>            hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_MISSING_INITIALIZER,
> -
>                    "Static variable cannot be initialized to 0 if it has object components.");

Actually, it seems that a static variable with objects is fine if it contains *only* objects.

[Giovanni Mascellani (＠giomasce)]

The MR is mostly good, and given that it's a bugfix it is also worthy of a freeze exception, but there a few issues to address.

[Francisco Casas (＠fcasas)]

On Mon Dec  5 13:04:05 2022 +0000, Giovanni Mascellani wrote:
> This seems to compile here, at least with profiles `ps_4_0`, `ps_5_0`
> and `ps_5_1`: https://shader-playground.timjones.io/4e83a24178541aea41b6cef402cd564f.

hmm, so it doesn't compile with 9.29.952.3111 (which I was using for testing) but it does with 10.0.10011.16384.

I guess we will have to add a way of having uninitialized objects, because in 10.0.10011.16384, the following shader:
```hlsl
static Texture2D tex;

float4 main() : sv_target { return tex.Load(int3(0, 0, 0)); }
```
results in:
```
error X4000: variable 'tex' used without having been completely initialized
```

[Francisco Casas (＠fcasas)]

On Mon Dec  5 13:04:05 2022 +0000, Giovanni Mascellani wrote:
> I think they should be formatted properly, even if they are really short.

Okay, there are some other tests with this shorter format in this file (and `hlsl-attributes.shader_test`) though.

[Francisco Casas (＠fcasas)]

On Mon Dec  5 13:04:05 2022 +0000, Giovanni Mascellani wrote:
> It's good to test for this failure, but we should probably also have a
> succeeding test for the usual case (having a static numeric variable,
> returning it and showing it is zero).

I wanted to avoid a nondeterministic test. If the test depends on an uninitialized value, it may succeed or fail randomly.

This one has the nicety that is constant folding that triggers the error iff the value is actually initialized to zero, otherwise, it has no way of knowing that the denominator will be zero.

[Francisco Casas (＠fcasas)]

On Mon Dec  5 13:04:06 2022 +0000, Giovanni Mascellani wrote:
> Actually, it seems that a static variable with objects is fine if it
> contains *only* objects.

Yep, seems to be the case in 10.0.10011.16384, but is not in 9.29.952.3111.

Should we go for the most permissive approach?

[Zebediah Figura (＠zfigura)]

On Mon Dec  5 15:28:01 2022 +0000, Francisco Casas wrote:
> Okay, there are some other tests with this shorter format in this file
> (and `hlsl-attributes.shader_test`) though.

Yeah, I don't think it's a big deal either way, personally, at least not in the tests, and not for very simple tests.

[Zebediah Figura (＠zfigura)]

On Mon Dec  5 15:24:56 2022 +0000, Francisco Casas wrote:
> hmm, so it doesn't compile with 9.29.952.3111 (which I was using for
> testing) but it does with 10.0.10011.16384.
> I guess we will have to add a way of having uninitialized objects,
> because in 10.0.10011.16384, the following shader:
> static Texture2D tex;
> float4 main() : sv_target { return tex.Load(int3(0, 0, 0)); }
>
> results in:
> error X4000: variable 'tex' used without having been completely initialized

Is that a version number on d3dcompiler_47?

[Zebediah Figura (＠zfigura)]

On Mon Dec  5 15:34:44 2022 +0000, Francisco Casas wrote:
> Yep, seems to be the case in 10.0.10011.16384, but is not in 9.29.952.3111.
> Should we go for the most permissive approach?

I would advocate for taking the most up-to-date behaviour (especially because it's easier, and more restrictive). We may at some point need to add a compiler switch to turn off that check, but it may make the most sense to just wait until a program actually needs it.

[Francisco Casas (＠fcasas)]

On Mon Dec  5 19:12:45 2022 +0000, Zebediah Figura wrote:
> I would advocate for taking the most up-to-date behaviour (especially
> because it's easier, and more restrictive). We may at some point need to
> add a compiler switch to turn off that check, but it may make the most
> sense to just wait until a program actually needs it.

The most up-to-date behavior (10.0.10011.16384) is also the most permissive, since it allows for uninitialized static variables with objects (to which a value can be initialized later) instead of just simply not allowing them.