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

[Rob Clark]

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

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