On 05/15/2016 03:23 PM, Rob Clark wrote:
> On Sun, May 15, 2016 at 8:52 AM, Eduardo Lima Mitev <el...@igalia.com> wrote:
>> On 05/14/2016 09:47 PM, Rob Clark wrote:
>>> From: Rob Clark <robcl...@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 <robcl...@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.
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 <el...@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 = {
>>>
>>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
