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

[Rob Clark]

On Fri, May 13, 2016 at 8:23 PM, Connor Abbott <cwabbott0 at gmail.com> wrote:
> 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.

fwiw, what I did was nir_validate constructs a hashtable mapping
offending object (currently instr or var, but I guess we could add
whatever) to assert string.. then passes that to nir_print which does
hashtable lookups as it prints instr/var/whatever and displays all the
failed assert msgs inline with the dump of the shader.  Seems to work
pretty well..

But I haven't yet done a rebase -i to squash that vs other various
interleaved unrelated fixup patches.. and still need to wire up the
error reporting for printing var's (which I didn't need yet for what I
was debugging) so I'll get around to cleaning that up and resending
sometime this weekend

BR,
-R


>>
>> 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