[Mesa-dev] Let's talk about -DDEBUG

[Erik Faye-Lund]

On Thu, 2018-12-13 at 10:46 -0800, Eric Anholt wrote:
> Dylan Baker <dylan at pnwbakers.com> writes:
> 
> > [ Unknown signature status ]
> > In the autotools discussion I've come to realize that we also need
> > to talk about
> > the -DDEBUG guard. It seems that there are two different uses, and
> > thus two
> > different asks about it:
> > 
> > - Nine (and RadeonSI?) use -DDEBUG to hide generic debugging
> > - NIR and Intel (at least) use -DDEBUG to hide really expensive
> > checks that are
> >   useful, but necessarily tank performance.
> > 
> > The first group would like -DDEBUG in debugoptimized builds, the
> > second
> > obviously doesn't.
> > 
> > Is the right solution to move the first group being !NDEBUG, or
> > would it be
> > better to split DEBUG into two different defines such as
> > DEBUG_MESSAGES and
> > EXPENSIVE_VALIDATION (paint the bikeshed whatever color you like),
> > with the
> > first for both debug and debugoptimized and the second only in
> > debug builds?
> 
> I would like to see NIR validation in debugoptimized builds (which is
> the build I use on a regular basis: "please catch all bugs you can at
> runtime with asserts, but don't waste CPU time by compiling with
> -O0");
> 

I'm starting to think that we should add explicit options (with
reasonable defaults based on ) for things like nir validation. That way
it'd be easy for anyone to pimp their buildtype. Meddling directly with
CFLAGS feels kinda hacky for something as useful like this.

Something like this?

---8<---
diff --git a/meson.build b/meson.build
index aba033387d3..da994e6ea87 100644
--- a/meson.build
+++ b/meson.build
@@ -734,6 +734,16 @@ if get_option('buildtype') == 'debug'
   pre_args += '-DDEBUG'
 endif
 
+# define NIR_VALIDATION if needed
+_nir_validation = get_option('nir-validation')
+if _nir_validation == 'auto'
+  if get_option('buildtype') == 'debug'
+    pre_args += '-DNIR_VALIDATION'
+  endif
+elif _nir_validation == 'true'
+  pre_args += '-DNIR_VALIDATION'
+endif
+
 if get_option('shader-cache')
   pre_args += '-DENABLE_SHADER_CACHE'
 elif with_amd_vk
diff --git a/meson_options.txt b/meson_options.txt
index c636b0a1099..9df20a6e080 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -318,3 +318,10 @@ option(
   choices : ['auto', 'true', 'false'],
   description : 'Enable VK_EXT_acquire_xlib_display.'
 )
+option(
+  'nir-validation',
+  type : 'combo',
+  value : 'auto',
+  choices : ['auto', 'true', 'false'],
+  description : 'Enable automatic validation of NIR. This has a non-
trivial performance penalty.'
+)
diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
index e8be2e101cc..d5609565bea 100644
--- a/src/compiler/nir/nir.h
+++ b/src/compiler/nir/nir.h
@@ -2727,7 +2727,7 @@ 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
+#ifndef NIR_VALIDATION
 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);
@@ -2768,7 +2768,7 @@ static inline void
nir_metadata_check_validation_flag(nir_shader *shader) { (voi
 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 /* NIR_VALIDATION */
 
 #define _PASS(pass, nir, do_pass) do {                               \
    do_pass                                                           \
---8<---