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

[Connor Abbott]

On Fri, May 13, 2016 at 9:07 PM, Rob Clark <robdclark at gmail.com> wrote:
> 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

Ok. I'd call them annotations instead of assert strings inside of
nir_print.c (since they're really more general than that) but
otherwise seems like a good idea.

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