[Mesa-dev] [RFC] nir/validate: on failure, dump shader w/ offending line annotated

[Connor Abbott]

On Fri, May 13, 2016 at 4:14 PM, Rob Clark <robdclark at gmail.com> wrote:
> On Fri, May 13, 2016 at 4:10 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>> On Fri, May 13, 2016 at 1:02 PM, Rob Clark <robdclark at gmail.com> wrote:
>>>
>>> From: Rob Clark <robclark at freedesktop.org>
>>>
>>> If we assert in nir_validate_shader(), print the shader with the
>>> offending instruction prefixed with "=>" to make it easier to find what
>>> part of the shader nir_validate is complaining about.
>>>
>>> Macro funny-business in nir_validate() was just to avoid changing a
>>> bazillion assert() lines to validate_assert() (or similar) for the point
>>> of an RFC ;-)
>>
>>
>> I love this idea.  I just wish it worked for more than just instructions.
>> It would also be fantastic if it were somehow able to print more than one
>> error.  Maybe something where we tie printing and validation together
>> somehow?  Just a thought.
>
> hmm, err_instr could easily become a void* (or array of void*?) to
> match var's/etc too..
>
> and nir_validate could easily keep a list of fails (maybe up to some
> threshold), and only assert at the end if num_errors > 0..
>
> That might be an easier way to go than merging the two existing
> passes..  although if I was starting from scratch merging the two
> might have been the better approach

If we want to show multiple failures, we probably want to display the
assertion failure inline when printing -- otherwise things might get
confusing when reading the output (which assertion goes with which
line?). We could have a way of adding annotation strings to an
instruction/variable/etc. when printing it, and then have nir_validate
use that. I'd imagine it might be useful for other things too, like
printing the results of an analysis pass for unit tests.

>
> BR,
> -R
>
>>>
>>> Example output: http://hastebin.com/raw/qorirayazu
>>> ---
>>>  src/compiler/nir/nir.h          |  1 +
>>>  src/compiler/nir/nir_print.c    | 14 +++++++++++++-
>>>  src/compiler/nir/nir_validate.c | 15 +++++++++++++++
>>>  3 files changed, 29 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
>>> index ade584c..6bb9fbe 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_err(nir_shader *shader, FILE *fp, nir_instr
>>> *instr);
>>>  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..3b25a49 100644
>>> --- a/src/compiler/nir/nir_print.c
>>> +++ b/src/compiler/nir/nir_print.c
>>> @@ -53,6 +53,8 @@ typedef struct {
>>>
>>>     /* an index used to make new non-conflicting names */
>>>     unsigned index;
>>> +
>>> +   nir_instr *err_instr;
>>>  } print_state;
>>>
>>>  static void
>>> @@ -916,6 +918,8 @@ print_block(nir_block *block, print_state *state,
>>> unsigned tabs)
>>>     free(preds);
>>>
>>>     nir_foreach_instr(instr, block) {
>>> +      if (instr == state->err_instr)
>>> +         fprintf(fp, "=>");
>>>        print_instr(instr, state, tabs);
>>>        fprintf(fp, "\n");
>>>     }
>>> @@ -1090,11 +1094,13 @@ destroy_print_state(print_state *state)
>>>  }
>>>
>>>  void
>>> -nir_print_shader(nir_shader *shader, FILE *fp)
>>> +nir_print_shader_err(nir_shader *shader, FILE *fp, nir_instr *instr)
>>>  {
>>>     print_state state;
>>>     init_print_state(&state, shader, fp);
>>>
>>> +   state.err_instr = instr;
>>> +
>>>     fprintf(fp, "shader: %s\n", gl_shader_stage_name(shader->stage));
>>>
>>>     if (shader->info.name)
>>> @@ -1144,6 +1150,12 @@ nir_print_shader(nir_shader *shader, FILE *fp)
>>>  }
>>>
>>>  void
>>> +nir_print_shader(nir_shader *shader, FILE *fp)
>>> +{
>>> +   nir_print_shader_err(shader, fp, NULL);
>>> +}
>>> +
>>> +void
>>>  nir_print_instr(const nir_instr *instr, FILE *fp)
>>>  {
>>>     print_state state = {
>>> diff --git a/src/compiler/nir/nir_validate.c
>>> b/src/compiler/nir/nir_validate.c
>>> index 84334d4..b47087f 100644
>>> --- a/src/compiler/nir/nir_validate.c
>>> +++ b/src/compiler/nir/nir_validate.c
>>> @@ -97,6 +97,21 @@ typedef struct {
>>>     struct hash_table *var_defs;
>>>  } validate_state;
>>>
>>> +
>>> +
>>> +static void
>>> +dump_assert(validate_state *state, const char *failed)
>>> +{
>>> +   fprintf(stderr, "validate failed: %s\n", failed);
>>> +   if (state->instr)
>>> +      nir_print_shader_err(state->shader, stderr, state->instr);
>>> +}
>>> +
>>> +#define __assert assert
>>> +#undef assert
>>> +#define assert(x) do { if (!(x)) { dump_assert(state, #x);
>>> __assert_fail(#x, __FILE__, __LINE__, __func__); } } while (0)
>>> +
>>> +
>>>  static void validate_src(nir_src *src, validate_state *state);
>>>
>>>  static void
>>> --
>>> 2.5.5
>>>
>>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev