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

[Rob Clark]

On Sun, May 15, 2016 at 1:53 PM, Eduardo Lima Mitev <elima at igalia.com> wrote:
> On 05/15/2016 03:23 PM, Rob Clark wrote:
>> On Sun, May 15, 2016 at 8:52 AM, Eduardo Lima Mitev <elima at igalia.com> wrote:
>>> 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).
>>
>> hmm..  I guess for showing error msgs (which is the one current use)
>> it made sense (at least to me) to show the message beneath..
>>
>> I guess I could do something like show "=>" at the beginning of the
>> actual line, and then the msg on the next line, if that would be less
>> confusing.
>>
>
> Ok, for error messages I agree that printing them below is the usual
> way. (I should have looked at your 2nd patch before commenting here).
> I would leave a blank line though, to separate the annotation from the
> next instruction. It would be more readable I think.

yeah, I like that idea.. I'll squash that in

BR,
-R

> Eduardo
>
>> And I suppose it is always an option to tweak things as we start
>> adding other uses for annotations.
>>
>> BR,
>> -R
>>
>>> 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 = {
>>>>
>>>
>>
>