Re: [Mesa-dev] [PATCH 03/11] panfrost/midgard: Flush undefineds to zero
> I don't think other drivers have had to do this. I've definitely > resisted stuff like that in nouveau in the past. Are you sure this is > necessary? Would be good to check if these tests pass or fail on > nouveau, for example. (Although by coincidence, they could be ending > up with zero values, of course...) It's not necessary, no, but... \shrug/ It is a good idea for us, at least; otherwise the undef will get a register allocated for it and then not uninitialize the register and we'll read back pseudorandom garbage. Rewriting to zero is legal and ensures that sort of impossible-to-debug bug never pops up, even if it's a dumb corner case. signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 03/11] panfrost/midgard: Flush undefineds to zero
It's a known upstream issue; it was taken off the mustpass list but... I have issues, ok? I just really wanted 100/100 passing :) I think it was 16. On Wed, Jul 10, 2019 at 03:48:35PM +0200, Erik Faye-Lund wrote: > On Wed, 2019-07-10 at 06:24 -0700, Alyssa Rosenzweig wrote: > > Fixes a buggy dEQP test. > > > > Maybe you could share which test this fixes, so someone can fix it? > > signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 03/11] panfrost/midgard: Flush undefineds to zero
On Wed, Jul 10, 2019 at 9:25 AM Alyssa Rosenzweig wrote: > > Fixes a buggy dEQP test. > > Signed-off-by: Alyssa Rosenzweig > --- > .../drivers/panfrost/ci/expected-failures.txt | 6 -- > src/gallium/drivers/panfrost/meson.build | 1 + > .../drivers/panfrost/midgard/compiler.h | 6 ++ > .../panfrost/midgard/midgard_compile.c| 4 + > .../panfrost/midgard/nir_undef_to_zero.c | 89 +++ > 5 files changed, 100 insertions(+), 6 deletions(-) > create mode 100644 src/gallium/drivers/panfrost/midgard/nir_undef_to_zero.c > > diff --git a/src/gallium/drivers/panfrost/ci/expected-failures.txt > b/src/gallium/drivers/panfrost/ci/expected-failures.txt > index 33059118b49..6f52773cc73 100644 > --- a/src/gallium/drivers/panfrost/ci/expected-failures.txt > +++ b/src/gallium/drivers/panfrost/ci/expected-failures.txt > @@ -256,12 +256,6 @@ dEQP-GLES2.functional.rasterization.limits.points > dEQP-GLES2.functional.shaders.builtin_variable.fragcoord_w > dEQP-GLES2.functional.shaders.preprocessor.predefined_macros.line_2_fragment > dEQP-GLES2.functional.shaders.preprocessor.predefined_macros.line_2_vertex > -dEQP-GLES2.functional.shaders.random.all_features.fragment.0 > -dEQP-GLES2.functional.shaders.random.all_features.fragment.16 > -dEQP-GLES2.functional.shaders.random.all_features.fragment.5 > -dEQP-GLES2.functional.shaders.random.all_features.fragment.6 > -dEQP-GLES2.functional.shaders.random.all_features.vertex.0 > -dEQP-GLES2.functional.shaders.random.all_features.vertex.17 > > dEQP-GLES2.functional.shaders.scoping.valid.local_variable_hides_function_parameter_fragment > > dEQP-GLES2.functional.shaders.scoping.valid.local_variable_hides_function_parameter_vertex > dEQP-GLES2.functional.shaders.struct.local.dynamic_loop_assignment_fragment > diff --git a/src/gallium/drivers/panfrost/midgard/nir_undef_to_zero.c > b/src/gallium/drivers/panfrost/midgard/nir_undef_to_zero.c > new file mode 100644 > index 000..aacecc17e9d > --- /dev/null > +++ b/src/gallium/drivers/panfrost/midgard/nir_undef_to_zero.c > + * Flushes undefined SSA values to a zero vector fo the appropriate component > + * count, to avoid undefined behaviour in the resulting shader. Not required > + * for conformance as use of uninitialized variables is explicitly left > + * undefined by the spec. Works around buggy apps, however. > + * > + * Call immediately after nir_opt_undef. If called before, larger > optimization > + * opportunities from the former pass will be missed. If called outside of an > + * optimization loop, constant propagation and algebraic optimizations won't > be > + * able to kick in to reduce stuff consuming the zero. I don't think other drivers have had to do this. I've definitely resisted stuff like that in nouveau in the past. Are you sure this is necessary? Would be good to check if these tests pass or fail on nouveau, for example. (Although by coincidence, they could be ending up with zero values, of course...) -ilia ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 03/11] panfrost/midgard: Flush undefineds to zero
On Wed, 2019-07-10 at 06:24 -0700, Alyssa Rosenzweig wrote: > Fixes a buggy dEQP test. > Maybe you could share which test this fixes, so someone can fix it? ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 03/11] panfrost/midgard: Flush undefineds to zero
On Wed, 2019-07-10 at 06:24 -0700, Alyssa Rosenzweig wrote:
> Fixes a buggy dEQP test.
>
> Signed-off-by: Alyssa Rosenzweig
> ---
> .../drivers/panfrost/ci/expected-failures.txt | 6 --
> src/gallium/drivers/panfrost/meson.build | 1 +
> .../drivers/panfrost/midgard/compiler.h | 6 ++
> .../panfrost/midgard/midgard_compile.c| 4 +
> .../panfrost/midgard/nir_undef_to_zero.c | 89
> +++
> 5 files changed, 100 insertions(+), 6 deletions(-)
> create mode 100644
> src/gallium/drivers/panfrost/midgard/nir_undef_to_zero.c
>
> diff --git a/src/gallium/drivers/panfrost/ci/expected-failures.txt
> b/src/gallium/drivers/panfrost/ci/expected-failures.txt
> index 33059118b49..6f52773cc73 100644
> --- a/src/gallium/drivers/panfrost/ci/expected-failures.txt
> +++ b/src/gallium/drivers/panfrost/ci/expected-failures.txt
> @@ -256,12 +256,6 @@ dEQP-
> GLES2.functional.rasterization.limits.points
> dEQP-GLES2.functional.shaders.builtin_variable.fragcoord_w
> dEQP-
> GLES2.functional.shaders.preprocessor.predefined_macros.line_2_fragme
> nt
> dEQP-
> GLES2.functional.shaders.preprocessor.predefined_macros.line_2_vertex
> -dEQP-GLES2.functional.shaders.random.all_features.fragment.0
> -dEQP-GLES2.functional.shaders.random.all_features.fragment.16
> -dEQP-GLES2.functional.shaders.random.all_features.fragment.5
> -dEQP-GLES2.functional.shaders.random.all_features.fragment.6
> -dEQP-GLES2.functional.shaders.random.all_features.vertex.0
> -dEQP-GLES2.functional.shaders.random.all_features.vertex.17
> dEQP-
> GLES2.functional.shaders.scoping.valid.local_variable_hides_function_
> parameter_fragment
> dEQP-
> GLES2.functional.shaders.scoping.valid.local_variable_hides_function_
> parameter_vertex
> dEQP-
> GLES2.functional.shaders.struct.local.dynamic_loop_assignment_fragmen
> t
> diff --git a/src/gallium/drivers/panfrost/meson.build
> b/src/gallium/drivers/panfrost/meson.build
> index 6b907f155ae..e1faa104f6b 100644
> --- a/src/gallium/drivers/panfrost/meson.build
> +++ b/src/gallium/drivers/panfrost/meson.build
> @@ -36,6 +36,7 @@ files_panfrost = files(
>'midgard/midgard_liveness.c',
>'midgard/midgard_ops.c',
>
> + 'midgard/nir_undef_to_zero.c',
>'midgard/nir_lower_blend.c',
>'midgard/nir_lower_framebuffer.c',
>'midgard/cppwrap.cpp',
> diff --git a/src/gallium/drivers/panfrost/midgard/compiler.h
> b/src/gallium/drivers/panfrost/midgard/compiler.h
> index 5fe1d80692c..820831d35dd 100644
> --- a/src/gallium/drivers/panfrost/midgard/compiler.h
> +++ b/src/gallium/drivers/panfrost/midgard/compiler.h
> @@ -444,4 +444,10 @@ void emit_binary_bundle(
> midgard_bundle *bundle,
> struct util_dynarray *emission,
> int next_tag);
> +
> +/* NIR stuff */
> +
> +bool
> +nir_undef_to_zero(nir_shader *shader);
> +
> #endif
> diff --git a/src/gallium/drivers/panfrost/midgard/midgard_compile.c
> b/src/gallium/drivers/panfrost/midgard/midgard_compile.c
> index 1e08e349eee..9c86f19fd06 100644
> --- a/src/gallium/drivers/panfrost/midgard/midgard_compile.c
> +++ b/src/gallium/drivers/panfrost/midgard/midgard_compile.c
> @@ -404,6 +404,8 @@ midgard_nir_lower_fdot2(nir_shader *shader)
> return progress;
> }
>
> +/* Flushes undefined values to zero */
> +
> static void
> optimise_nir(nir_shader *nir)
> {
> @@ -464,6 +466,8 @@ optimise_nir(nir_shader *nir)
> }
>
> NIR_PASS(progress, nir, nir_opt_undef);
> +NIR_PASS(progress, nir, nir_undef_to_zero);
> +
> NIR_PASS(progress, nir, nir_opt_loop_unroll,
> nir_var_shader_in |
> nir_var_shader_out |
> diff --git a/src/gallium/drivers/panfrost/midgard/nir_undef_to_zero.c
> b/src/gallium/drivers/panfrost/midgard/nir_undef_to_zero.c
> new file mode 100644
> index 000..aacecc17e9d
> --- /dev/null
> +++ b/src/gallium/drivers/panfrost/midgard/nir_undef_to_zero.c
> @@ -0,0 +1,89 @@
> +/*
> + * Copyright (C) 2019 Collabora, Ltd.
> + *
> + * Permission is hereby granted, free of charge, to any person
> obtaining a
> + * copy of this software and associated documentation files (the
> "Software"),
> + * to deal in the Software without restriction, including without
> limitation
> + * the rights to use, copy, modify, merge, publish, distribute,
> sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom
> the
> + * Software is furnished to do so, subject to the following
> conditions:
> + *
> + * The above copyright notice and this permission notice (including
> the next
> + * paragraph) shall be included in all copies or substantial
> portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO
> EVENT SHALL
> + * THE
[Mesa-dev] [PATCH 03/11] panfrost/midgard: Flush undefineds to zero
Fixes a buggy dEQP test.
Signed-off-by: Alyssa Rosenzweig
---
.../drivers/panfrost/ci/expected-failures.txt | 6 --
src/gallium/drivers/panfrost/meson.build | 1 +
.../drivers/panfrost/midgard/compiler.h | 6 ++
.../panfrost/midgard/midgard_compile.c| 4 +
.../panfrost/midgard/nir_undef_to_zero.c | 89 +++
5 files changed, 100 insertions(+), 6 deletions(-)
create mode 100644 src/gallium/drivers/panfrost/midgard/nir_undef_to_zero.c
diff --git a/src/gallium/drivers/panfrost/ci/expected-failures.txt
b/src/gallium/drivers/panfrost/ci/expected-failures.txt
index 33059118b49..6f52773cc73 100644
--- a/src/gallium/drivers/panfrost/ci/expected-failures.txt
+++ b/src/gallium/drivers/panfrost/ci/expected-failures.txt
@@ -256,12 +256,6 @@ dEQP-GLES2.functional.rasterization.limits.points
dEQP-GLES2.functional.shaders.builtin_variable.fragcoord_w
dEQP-GLES2.functional.shaders.preprocessor.predefined_macros.line_2_fragment
dEQP-GLES2.functional.shaders.preprocessor.predefined_macros.line_2_vertex
-dEQP-GLES2.functional.shaders.random.all_features.fragment.0
-dEQP-GLES2.functional.shaders.random.all_features.fragment.16
-dEQP-GLES2.functional.shaders.random.all_features.fragment.5
-dEQP-GLES2.functional.shaders.random.all_features.fragment.6
-dEQP-GLES2.functional.shaders.random.all_features.vertex.0
-dEQP-GLES2.functional.shaders.random.all_features.vertex.17
dEQP-GLES2.functional.shaders.scoping.valid.local_variable_hides_function_parameter_fragment
dEQP-GLES2.functional.shaders.scoping.valid.local_variable_hides_function_parameter_vertex
dEQP-GLES2.functional.shaders.struct.local.dynamic_loop_assignment_fragment
diff --git a/src/gallium/drivers/panfrost/meson.build
b/src/gallium/drivers/panfrost/meson.build
index 6b907f155ae..e1faa104f6b 100644
--- a/src/gallium/drivers/panfrost/meson.build
+++ b/src/gallium/drivers/panfrost/meson.build
@@ -36,6 +36,7 @@ files_panfrost = files(
'midgard/midgard_liveness.c',
'midgard/midgard_ops.c',
+ 'midgard/nir_undef_to_zero.c',
'midgard/nir_lower_blend.c',
'midgard/nir_lower_framebuffer.c',
'midgard/cppwrap.cpp',
diff --git a/src/gallium/drivers/panfrost/midgard/compiler.h
b/src/gallium/drivers/panfrost/midgard/compiler.h
index 5fe1d80692c..820831d35dd 100644
--- a/src/gallium/drivers/panfrost/midgard/compiler.h
+++ b/src/gallium/drivers/panfrost/midgard/compiler.h
@@ -444,4 +444,10 @@ void emit_binary_bundle(
midgard_bundle *bundle,
struct util_dynarray *emission,
int next_tag);
+
+/* NIR stuff */
+
+bool
+nir_undef_to_zero(nir_shader *shader);
+
#endif
diff --git a/src/gallium/drivers/panfrost/midgard/midgard_compile.c
b/src/gallium/drivers/panfrost/midgard/midgard_compile.c
index 1e08e349eee..9c86f19fd06 100644
--- a/src/gallium/drivers/panfrost/midgard/midgard_compile.c
+++ b/src/gallium/drivers/panfrost/midgard/midgard_compile.c
@@ -404,6 +404,8 @@ midgard_nir_lower_fdot2(nir_shader *shader)
return progress;
}
+/* Flushes undefined values to zero */
+
static void
optimise_nir(nir_shader *nir)
{
@@ -464,6 +466,8 @@ optimise_nir(nir_shader *nir)
}
NIR_PASS(progress, nir, nir_opt_undef);
+NIR_PASS(progress, nir, nir_undef_to_zero);
+
NIR_PASS(progress, nir, nir_opt_loop_unroll,
nir_var_shader_in |
nir_var_shader_out |
diff --git a/src/gallium/drivers/panfrost/midgard/nir_undef_to_zero.c
b/src/gallium/drivers/panfrost/midgard/nir_undef_to_zero.c
new file mode 100644
index 000..aacecc17e9d
--- /dev/null
+++ b/src/gallium/drivers/panfrost/midgard/nir_undef_to_zero.c
@@ -0,0 +1,89 @@
+/*
+ * Copyright (C) 2019 Collabora, Ltd.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors (Collabora):
+ * Alyssa Rosenzweig
+ */
+
+/**
+ * @file
+ *
