[RFC] nir: call nir_validate_shader in debug but not debugoptimized builds

Submitted by Marek Olšák on May 11, 2019, 1:15 a.m.

Details

Message ID 20190511011547.10886-1-maraeo@gmail.com
State New
Headers show
Series "nir: call nir_validate_shader in debug but not debugoptimized builds" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Marek Olšák May 11, 2019, 1:15 a.m.
From: Marek Olšák <marek.olsak@amd.com>

This reverts commit 7b85b9b8773b119360a31b66b321ae560a77cb6d.
---
 src/compiler/nir/nir.h          | 8 ++++----
 src/compiler/nir/nir_metadata.c | 2 +-
 src/compiler/nir/nir_validate.c | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
index 8441c9f26c5..6ee733efa75 100644
--- a/src/compiler/nir/nir.h
+++ b/src/compiler/nir/nir.h
@@ -35,23 +35,23 @@ 
 #include "util/ralloc.h"
 #include "util/set.h"
 #include "util/bitscan.h"
 #include "util/bitset.h"
 #include "util/macros.h"
 #include "compiler/nir_types.h"
 #include "compiler/shader_enums.h"
 #include "compiler/shader_info.h"
 #include <stdio.h>
 
-#ifndef NDEBUG
+#ifdef DEBUG
 #include "util/debug.h"
-#endif /* NDEBUG */
+#endif /* DEBUG */
 
 #include "nir_opcodes.h"
 
 #if defined(_WIN32) && !defined(snprintf)
 #define snprintf _snprintf
 #endif
 
 #ifdef __cplusplus
 extern "C" {
 #endif
@@ -2875,21 +2875,21 @@  void nir_print_instr(const nir_instr *instr, FILE *fp);
 void nir_print_deref(const nir_deref_instr *deref, FILE *fp);
 
 nir_shader *nir_shader_clone(void *mem_ctx, const nir_shader *s);
 nir_function_impl *nir_function_impl_clone(nir_shader *shader,
                                            const nir_function_impl *fi);
 nir_constant *nir_constant_clone(const nir_constant *c, nir_variable *var);
 nir_variable *nir_variable_clone(const nir_variable *c, nir_shader *shader);
 
 nir_shader *nir_shader_serialize_deserialize(void *mem_ctx, nir_shader *s);
 
-#ifndef NDEBUG
+#ifdef DEBUG
 void nir_validate_shader(nir_shader *shader, const char *when);
 void nir_metadata_set_validation_flag(nir_shader *shader);
 void nir_metadata_check_validation_flag(nir_shader *shader);
 
 static inline bool
 should_skip_nir(const char *name)
 {
    static const char *list = NULL;
    if (!list) {
       /* Comma separated list of names to skip. */
@@ -2934,21 +2934,21 @@  should_print_nir(void)
    return should_print;
 }
 #else
 static inline void nir_validate_shader(nir_shader *shader, const char *when) { (void) shader; (void)when; }
 static inline void nir_metadata_set_validation_flag(nir_shader *shader) { (void) shader; }
 static inline void nir_metadata_check_validation_flag(nir_shader *shader) { (void) shader; }
 static inline bool should_skip_nir(UNUSED const char *pass_name) { return false; }
 static inline bool should_clone_nir(void) { return false; }
 static inline bool should_serialize_deserialize_nir(void) { return false; }
 static inline bool should_print_nir(void) { return false; }
-#endif /* NDEBUG */
+#endif /* DEBUG */
 
 #define _PASS(pass, nir, do_pass) do {                               \
    if (should_skip_nir(#pass)) {                                     \
       printf("skipping %s\n", #pass);                                \
       break;                                                         \
    }                                                                 \
    do_pass                                                           \
    nir_validate_shader(nir, "after " #pass);                         \
    if (should_clone_nir()) {                                         \
       nir_shader *clone = nir_shader_clone(ralloc_parent(nir), nir); \
diff --git a/src/compiler/nir/nir_metadata.c b/src/compiler/nir/nir_metadata.c
index e681ba34f75..f71cf432b70 100644
--- a/src/compiler/nir/nir_metadata.c
+++ b/src/compiler/nir/nir_metadata.c
@@ -52,21 +52,21 @@  nir_metadata_require(nir_function_impl *impl, nir_metadata required, ...)
 
    impl->valid_metadata |= required;
 }
 
 void
 nir_metadata_preserve(nir_function_impl *impl, nir_metadata preserved)
 {
    impl->valid_metadata &= preserved;
 }
 
-#ifndef NDEBUG
+#ifdef DEBUG
 /**
  * Make sure passes properly invalidate metadata (part 1).
  *
  * Call this before running a pass to set a bogus metadata flag, which will
  * only be preserved if the pass forgets to call nir_metadata_preserve().
  */
 void
 nir_metadata_set_validation_flag(nir_shader *shader)
 {
    nir_foreach_function(function, shader) {
diff --git a/src/compiler/nir/nir_validate.c b/src/compiler/nir/nir_validate.c
index 7746c391abc..a57b78e97c0 100644
--- a/src/compiler/nir/nir_validate.c
+++ b/src/compiler/nir/nir_validate.c
@@ -29,21 +29,21 @@ 
 #include "c11/threads.h"
 #include <assert.h>
 
 /*
  * This file checks for invalid IR indicating a bug somewhere in the compiler.
  */
 
 /* Since this file is just a pile of asserts, don't bother compiling it if
  * we're not building a debug build.
  */
-#ifndef NDEBUG
+#ifdef DEBUG
 
 /*
  * Per-register validation state.
  */
 
 typedef struct {
    /*
     * equivalent to the uses and defs in nir_register, but built up by the
     * validator. At the end, we verify that the sets have the same entries.
     */

Comments

Please, no.  You could make a case for changing the default in 
debugoptimized builds (which I would still be against) but we should 
definitely still compile it. Why is it so hard to set NIR_VALIDATE=0 when 
you really care about compiler times?

--Jason


On May 10, 2019 20:15:56 Marek Olšák <maraeo@gmail.com> wrote:

> From: Marek Olšák <marek.olsak@amd.com>
>
>
> This reverts commit 7b85b9b8773b119360a31b66b321ae560a77cb6d.
> ---
> src/compiler/nir/nir.h          | 8 ++++----
> src/compiler/nir/nir_metadata.c | 2 +-
> src/compiler/nir/nir_validate.c | 2 +-
> 3 files changed, 6 insertions(+), 6 deletions(-)
>
>
> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
> index 8441c9f26c5..6ee733efa75 100644
> --- a/src/compiler/nir/nir.h
> +++ b/src/compiler/nir/nir.h
> @@ -35,23 +35,23 @@
> #include "util/ralloc.h"
> #include "util/set.h"
> #include "util/bitscan.h"
> #include "util/bitset.h"
> #include "util/macros.h"
> #include "compiler/nir_types.h"
> #include "compiler/shader_enums.h"
> #include "compiler/shader_info.h"
> #include <stdio.h>
>
> -#ifndef NDEBUG
> +#ifdef DEBUG
> #include "util/debug.h"
> -#endif /* NDEBUG */
> +#endif /* DEBUG */
>
> #include "nir_opcodes.h"
>
> #if defined(_WIN32) && !defined(snprintf)
> #define snprintf _snprintf
> #endif
>
> #ifdef __cplusplus
> extern "C" {
> #endif
> @@ -2875,21 +2875,21 @@ void nir_print_instr(const nir_instr *instr, FILE *fp);
> void nir_print_deref(const nir_deref_instr *deref, FILE *fp);
>
> nir_shader *nir_shader_clone(void *mem_ctx, const nir_shader *s);
> nir_function_impl *nir_function_impl_clone(nir_shader *shader,
>                                            const nir_function_impl *fi);
> nir_constant *nir_constant_clone(const nir_constant *c, nir_variable *var);
> nir_variable *nir_variable_clone(const nir_variable *c, nir_shader *shader);
>
> nir_shader *nir_shader_serialize_deserialize(void *mem_ctx, nir_shader *s);
>
> -#ifndef NDEBUG
> +#ifdef DEBUG
> void nir_validate_shader(nir_shader *shader, const char *when);
> void nir_metadata_set_validation_flag(nir_shader *shader);
> void nir_metadata_check_validation_flag(nir_shader *shader);
>
> static inline bool
> should_skip_nir(const char *name)
> {
>    static const char *list = NULL;
>    if (!list) {
>       /* Comma separated list of names to skip. */
> @@ -2934,21 +2934,21 @@ should_print_nir(void)
>    return should_print;
> }
> #else
> static inline void nir_validate_shader(nir_shader *shader, const char 
> *when) { (void) shader; (void)when; }
> static inline void nir_metadata_set_validation_flag(nir_shader *shader) { 
> (void) shader; }
> static inline void nir_metadata_check_validation_flag(nir_shader *shader) { 
> (void) shader; }
> static inline bool should_skip_nir(UNUSED const char *pass_name) { return 
> false; }
> static inline bool should_clone_nir(void) { return false; }
> static inline bool should_serialize_deserialize_nir(void) { return false; }
> static inline bool should_print_nir(void) { return false; }
> -#endif /* NDEBUG */
> +#endif /* DEBUG */
>
> #define _PASS(pass, nir, do_pass) do {                               \
>    if (should_skip_nir(#pass)) {                                     \
>       printf("skipping %s\n", #pass);                                \
>       break;                                                         \
>    }                                                                 \
>    do_pass                                                           \
>    nir_validate_shader(nir, "after " #pass);                         \
>    if (should_clone_nir()) {                                         \
>       nir_shader *clone = nir_shader_clone(ralloc_parent(nir), nir); \
> diff --git a/src/compiler/nir/nir_metadata.c b/src/compiler/nir/nir_metadata.c
> index e681ba34f75..f71cf432b70 100644
> --- a/src/compiler/nir/nir_metadata.c
> +++ b/src/compiler/nir/nir_metadata.c
> @@ -52,21 +52,21 @@ nir_metadata_require(nir_function_impl *impl, 
> nir_metadata required, ...)
>
>    impl->valid_metadata |= required;
> }
>
> void
> nir_metadata_preserve(nir_function_impl *impl, nir_metadata preserved)
> {
>    impl->valid_metadata &= preserved;
> }
>
> -#ifndef NDEBUG
> +#ifdef DEBUG
> /**
>  * Make sure passes properly invalidate metadata (part 1).
>  *
>  * Call this before running a pass to set a bogus metadata flag, which will
>  * only be preserved if the pass forgets to call nir_metadata_preserve().
>  */
> void
> nir_metadata_set_validation_flag(nir_shader *shader)
> {
>    nir_foreach_function(function, shader) {
> diff --git a/src/compiler/nir/nir_validate.c b/src/compiler/nir/nir_validate.c
> index 7746c391abc..a57b78e97c0 100644
> --- a/src/compiler/nir/nir_validate.c
> +++ b/src/compiler/nir/nir_validate.c
> @@ -29,21 +29,21 @@
> #include "c11/threads.h"
> #include <assert.h>
>
> /*
>  * This file checks for invalid IR indicating a bug somewhere in the compiler.
>  */
>
> /* Since this file is just a pile of asserts, don't bother compiling it if
>  * we're not building a debug build.
>  */
> -#ifndef NDEBUG
> +#ifdef DEBUG
>
> /*
>  * Per-register validation state.
>  */
>
> typedef struct {
>    /*
>     * equivalent to the uses and defs in nir_register, but built up by the
>     * validator. At the end, we verify that the sets have the same entries.
>     */
> --
> 2.17.1
>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev