[Mesa-dev] [PATCH 1/2] nir/print: add support for print annotations

[Eduardo Lima Mitev]

On 05/14/2016 09:47 PM, Rob Clark wrote:
> From: Rob Clark <robclark at freedesktop.org>
> 
> Caller can pass a hashtable mapping NIR object (currently instr or var,
> but I guess others could be added as needed) to annotation msg to print
> inline with the shader dump.  As the annotation msg is printed, it is
> removed from the hashtable to give the caller a way to know about any
> unassociated msgs.
> 
> This is used in the next patch, for nir_validate to try to associate
> error msgs to nir_print dump.
> 
> Signed-off-by: Rob Clark <robclark at freedesktop.org>
> ---
>  src/compiler/nir/nir.h       |  1 +
>  src/compiler/nir/nir_print.c | 41 ++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
> index ade584c..5f2cc8e 100644
> --- a/src/compiler/nir/nir.h
> +++ b/src/compiler/nir/nir.h
> @@ -2196,6 +2196,7 @@ unsigned nir_index_instrs(nir_function_impl *impl);
>  void nir_index_blocks(nir_function_impl *impl);
>  
>  void nir_print_shader(nir_shader *shader, FILE *fp);
> +void nir_print_shader_annotated(nir_shader *shader, FILE *fp, struct hash_table *errors);
>  void nir_print_instr(const nir_instr *instr, FILE *fp);
>  
>  nir_shader *nir_shader_clone(void *mem_ctx, const nir_shader *s);
> diff --git a/src/compiler/nir/nir_print.c b/src/compiler/nir/nir_print.c
> index a36561e..70ed73f 100644
> --- a/src/compiler/nir/nir_print.c
> +++ b/src/compiler/nir/nir_print.c
> @@ -53,8 +53,28 @@ typedef struct {
>  
>     /* an index used to make new non-conflicting names */
>     unsigned index;
> +
> +   /**
> +    * Optional table of annotations mapping nir object
> +    * (such as instr or var) to message to print.
> +    */
> +   struct hash_table *annotations;
>  } print_state;
>  
> +static const char *
> +get_annotation(print_state *state, void *obj)
> +{
> +   if (!state->annotations)
> +      return NULL;
> +
> +   struct hash_entry *entry = _mesa_hash_table_search(state->annotations, obj);
> +   if (entry) {
> +      _mesa_hash_table_remove(state->annotations, entry);
> +      return entry->data;
> +   }
> +   return NULL;
> +}
> +
>  static void
>  print_register(nir_register *reg, print_state *state)
>  {
> @@ -413,6 +433,11 @@ print_var_decl(nir_variable *var, print_state *state)
>     }
>  
>     fprintf(fp, "\n");
> +
> +   const char *note = get_annotation(state, var);
> +   if (note) {
> +      fprintf(stderr, "%s\n", note);
> +   }
>  }
>

This will print the annotation in a new line below the annotated
instruction. Isn't this confusing? I would expect an annotation to
affect the instruction immediately below it, or in the same line, not
the one above. (thinking on code comments, for example).

Other that that, patch looks fine, so with the annotation format
clarified, this would be:

Reviewed-by: Eduardo Lima Mitev <elima at igalia.com>

>  static void
> @@ -918,6 +943,11 @@ print_block(nir_block *block, print_state *state, unsigned tabs)
>     nir_foreach_instr(instr, block) {
>        print_instr(instr, state, tabs);
>        fprintf(fp, "\n");
> +
> +      const char *note = get_annotation(state, instr);
> +      if (note) {
> +         fprintf(stderr, "%s\n", note);
> +      }
>     }
>  
>     print_tabs(tabs, fp);
> @@ -1090,11 +1120,14 @@ destroy_print_state(print_state *state)
>  }
>  
>  void
> -nir_print_shader(nir_shader *shader, FILE *fp)
> +nir_print_shader_annotated(nir_shader *shader, FILE *fp,
> +                           struct hash_table *annotations)
>  {
>     print_state state;
>     init_print_state(&state, shader, fp);
>  
> +   state.annotations = annotations;
> +
>     fprintf(fp, "shader: %s\n", gl_shader_stage_name(shader->stage));
>  
>     if (shader->info.name)
> @@ -1144,6 +1177,12 @@ nir_print_shader(nir_shader *shader, FILE *fp)
>  }
>  
>  void
> +nir_print_shader(nir_shader *shader, FILE *fp)
> +{
> +   nir_print_shader_annotated(shader, fp, NULL);
> +}
> +
> +void
>  nir_print_instr(const nir_instr *instr, FILE *fp)
>  {
>     print_state state = {
>