[Mesa-dev] [RFC] nir/validate: on failure, dump shader w/ offending line annotated
Rob Clark
robdclark at gmail.com
Sat May 14 01:17:49 UTC 2016
On Fri, May 13, 2016 at 9:13 PM, Connor Abbott <cwabbott0 at gmail.com> wrote:
> 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.
right, I'll concat the "error:" part of the msg in nir_validate, and
rename things a bit to generalize.
BR,
-R
>>
>> 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
More information about the mesa-dev
mailing list