[Mesa-dev] [PATCH 8/8] i965/nir: Validate that NIR passes call nir_metadata_preserve().

[Kenneth Graunke]

Failing to call nir_metadata_preserve() can have nasty consequences:
some pass breaks dominance information, but leaves it marked as valid,
causing some subsequent pass to go haywire and probably crash.

This pass adds a simple validation mechanism to ensure passes handle
this properly.  We add a new bogus metadata flag that isn't used for
anything in particular, set it before each pass, and ensure it *isn't*
still set after the pass.  nir_metadata_preserve will reset the flag,
so correct passes will work, and bad passes will assert fail.

(I would have made these functions static inline, but nir.h is included
in C++, so we can't bit-or enums without lots of casting...)

Thanks to Dylan Baker for the idea.

Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
---
 src/glsl/nir/nir.h                  |  5 +++++
 src/glsl/nir/nir_metadata.c         | 36 ++++++++++++++++++++++++++++++++++++
 src/mesa/drivers/dri/i965/brw_nir.c | 10 +++++++---
 3 files changed, 48 insertions(+), 3 deletions(-)

One slightly ugly aspect is that we leave the bogus flag set if a pass
doesn't make any progress (i.e. doesn't alter the tree), as it won't
necessarily call nir_metadata_preserve in that case.  This should be
pretty harmless, though...

diff --git a/src/glsl/nir/nir.h b/src/glsl/nir/nir.h
index 874a039..1ab740a 100644
--- a/src/glsl/nir/nir.h
+++ b/src/glsl/nir/nir.h
@@ -1311,6 +1311,7 @@ typedef enum {
    nir_metadata_block_index = 0x1,
    nir_metadata_dominance = 0x2,
    nir_metadata_live_variables = 0x4,
+   nir_metadata_not_properly_reset = 0x8,
 } nir_metadata;
 
 typedef struct {
@@ -1880,8 +1881,12 @@ void nir_print_instr(const nir_instr *instr, FILE *fp);
 
 #ifdef DEBUG
 void nir_validate_shader(nir_shader *shader);
+void nir_metadata_set_validation_flag(nir_shader *shader);
+void nir_metadata_check_validation_flag(nir_shader *shader);
 #else
 static inline void nir_validate_shader(nir_shader *shader) { (void) shader; }
+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; }
 #endif /* DEBUG */
 
 void nir_calc_dominance_impl(nir_function_impl *impl);
diff --git a/src/glsl/nir/nir_metadata.c b/src/glsl/nir/nir_metadata.c
index a03e124..0b0159f 100644
--- a/src/glsl/nir/nir_metadata.c
+++ b/src/glsl/nir/nir_metadata.c
@@ -52,3 +52,39 @@ nir_metadata_preserve(nir_function_impl *impl, nir_metadata preserved)
 {
    impl->valid_metadata &= preserved;
 }
+
+#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_overload(shader, overload) {
+      if (overload->impl) {
+         overload->impl->valid_metadata |= nir_metadata_not_properly_reset;
+      }
+   }
+}
+
+/**
+ * Make sure passes properly invalidate metadata (part 2).
+ *
+ * Call this after a pass makes progress to verify that the bogus metadata set by
+ * the earlier function was properly thrown away.  Note that passes may not call
+ * nir_metadata_preserve() if they don't actually make any changes at all.
+ */
+void
+nir_metadata_check_validation_flag(nir_shader *shader)
+{
+   nir_foreach_overload(shader, overload) {
+      if (overload->impl) {
+         assert(!(overload->impl->valid_metadata &
+                  nir_metadata_not_properly_reset));
+      }
+   }
+}
+#endif
diff --git a/src/mesa/drivers/dri/i965/brw_nir.c b/src/mesa/drivers/dri/i965/brw_nir.c
index e71a7d1..7b72d36 100644
--- a/src/mesa/drivers/dri/i965/brw_nir.c
+++ b/src/mesa/drivers/dri/i965/brw_nir.c
@@ -143,9 +143,13 @@ brw_nir_lower_outputs(nir_shader *nir, bool is_scalar)
    this_progress;             \
 }))
 
-#define OPT(pass, ...) _OPT(                 \
-   this_progress = pass(nir ,##__VA_ARGS__); \
-   progress = progress || this_progress;     \
+#define OPT(pass, ...) _OPT(                   \
+   nir_metadata_set_validation_flag(nir);      \
+   this_progress = pass(nir ,##__VA_ARGS__);   \
+   if (this_progress) {                        \
+      progress = true;                         \
+      nir_metadata_check_validation_flag(nir); \
+   }                                           \
 )
 
 #define OPT_V(pass, ...) _OPT( \
-- 
2.6.2