[Mesa-dev] [PATCH 6/9] i965: Add annotation_insert_error() and support for printing errors.

[Matt Turner]

On Tue, Nov 3, 2015 at 9:47 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On Wednesday, October 21, 2015 03:58:14 PM Matt Turner wrote:
>> Will allow annotations to contain error messages (indicating an
>> instruction violates a rule for instance) that are printed after the
>> disassembly of the block.
>> ---
>>  src/mesa/drivers/dri/i965/intel_asm_annotation.c | 60 ++++++++++++++++++++++++
>>  src/mesa/drivers/dri/i965/intel_asm_annotation.h |  7 +++
>>  2 files changed, 67 insertions(+)
>>
>> diff --git a/src/mesa/drivers/dri/i965/intel_asm_annotation.c b/src/mesa/drivers/dri/i965/intel_asm_annotation.c
>> index 58830db..eaee386 100644
>> --- a/src/mesa/drivers/dri/i965/intel_asm_annotation.c
>> +++ b/src/mesa/drivers/dri/i965/intel_asm_annotation.c
>> @@ -69,6 +69,10 @@ dump_assembly(void *assembly, int num_annotations, struct annotation *annotation
>>
>>        brw_disassemble(devinfo, assembly, start_offset, end_offset, stderr);
>>
>> +      if (annotation[i].error) {
>> +         fputs(annotation[i].error, stderr);
>> +      }
>> +
>>        if (annotation[i].block_end) {
>>           fprintf(stderr, "   END B%d", annotation[i].block_end->num);
>>           foreach_list_typed(struct bblock_link, successor_link, link,
>> @@ -152,3 +156,59 @@ annotation_finalize(struct annotation_info *annotation,
>>     }
>>     annotation->ann[annotation->ann_count].offset = next_inst_offset;
>>  }
>> +
>> +void
>> +annotation_insert_error(struct annotation_info *annotation, unsigned offset,
>> +                        const char *error)
>> +{
>> +   struct annotation *ann = NULL;
>> +
>> +   if (!annotation->ann_count)
>> +      return;
>
> If I'm reading this correctly, it means that we won't report any errors
> if there are no annotations.  Shouldn't we instead insert a new
> annotation in this case?  Something like:

ann_count is nonzero iff we're printing the disassembly. ann_count
counts the number of "annotation blocks", not the number of
source-level/IR annotations.

And if we're not printing the disassembly, then yeah, we won't print
any errors... because where and how would we print them?

>    if (annotation->ann_count == 0) {
>       struct annotation *ann = &annotation->ann[0];
>       annotation->ann_count++;
>       ann->offset = 0;
>       ann->block_start = NULL;
>       ann->block_end = NULL;
>       ann->ir = NULL;
>       ann->annotation = NULL;
>       ann->error = ralloc_strdup(annotation->mem_ctx, error);
>       return;
>    }
>
>> +
>> +   /* We may have to split an annotation, so ensure we have enough space
>> +    * allocated for that case up front.
>> +    */
>> +   if (annotation->ann_size <= annotation->ann_count) {
>> +      int old_size = annotation->ann_size;
>> +      annotation->ann_size = MAX2(1024, annotation->ann_size * 2);
>> +      annotation->ann = reralloc(annotation->mem_ctx, annotation->ann,
>> +                                 struct annotation, annotation->ann_size);
>> +      if (!annotation->ann)
>> +         return;
>> +
>> +      memset(annotation->ann + old_size, 0,
>> +             (annotation->ann_size - old_size) * sizeof(struct annotation));
>> +   }
>
> I agree with Topi, let's use a helper function, such as:
>
> /**
>  * Make sure the annotation->ann[] array has room for one more element.
>  */
> static void
> grow_annotation_array(struct annotation_info *annotation)
> {
>    ...
> }
>
>> +
>> +   for (int i = 0; i <= annotation->ann_count; i++) {
>
> Tricky...you're relying on the fact that annotation_finalize() has made
> ann[ann_count] exist and have an offset equal to the end of the program.
> So, there's a sentinel annotation of sorts, which makes <= okay (as
> opposed to the obvious choice of <), and also guarantees that you'll
> actually find an element whose offset is larger.

You want to find the one that includes the instruction at <offset>, so
you skip things that are "not greater", i.e. <=, than the offset.

> But this isn't safe when i = 0, as cur will be out of bounds...

I don't think so. The code as-is might be tricky, but I think it's correct --

>> +      if (annotation->ann[i].offset <= offset)
>> +         continue;

For i = 0, annotation->ann[i].offset is always 0 (or the start of the
SIMD16 program, I suppose). In both cases, <offset> cannot be less
than that value. Knowing that, maybe the loop should just begin at i =
1.

At least, I think. It's been more than two weeks since I've really
thought about this :(

>> +
>> +      struct annotation *cur = &annotation->ann[i - 1];
>> +      struct annotation *next = &annotation->ann[i];
>
> Okay, having "cur" be the (i - 1)th element and "next" be the ith
> element is just mean :(  That's not what those mean.
>
>> +      ann = cur;
>> +
>> +      if (offset + sizeof(brw_inst) != next->offset) {
>> +         memmove(next, cur,
>> +                 (annotation->ann_count - i + 2) * sizeof(struct annotation));
>> +         cur->error = NULL;
>> +         cur->error_length = 0;
>> +         cur->block_end = NULL;
>> +         next->offset = offset + sizeof(brw_inst);
>> +         next->block_start = NULL;
>> +         annotation->ann_count++;
>> +      }
>> +      break;
>> +   }
>
> I think this would be easier to follow:
>
>    /* We want to insert the error comment /after/ the instruction. */
>    unsigned insertion_point = offset + sizeof(brw_inst);
>
>    /* Note that annotation_finalize() has placed a sentinel annotation
>     * at annotation->ann[annotation->ann_count] with an offset that is
>     * the end of the program.  So we're guaranteed to find an entry.
>     */
>    for (int i = 0; i <= annotation->ann_count; i++) {
>       struct annotation *cur = &annotation->ann[i];
>
>       /* If the current annotation begins exactly where we want to
>        * insert the error, we can simply use it.
>        */
>       if (cur->offset == insertion_point) {
>          ann = cur;
>          break;
>       }
>
>       /* If the current annotation starts beyond our insertion point,
>        * stop.  Insert a new annotation before cur, shifting the rest
>        * of the array over to make room.
>        */
>       if (cur->offset > insertion_point) {
>          cur->block_start = NULL; // XXX: why?

I have no idea why... my code didn't do this :)

The idea in setting block_start/end to null is that when splitting the
annotation block, the second one can't start a basic block and the
first can't end a basic block.

>          memmove(cur + 1, cur,
>                  (annotation->ann_count - i + 2) * sizeof(struct annotation));
>          ann = cur;
>          ann->error = NULL;
>          ann->error_length = 0;
>          ann->block_end = NULL;
>          ann->offset = insertion_point;
>          annotation->ann_count++;
>          break;
>       }
>    }
>
> Actually, if you make annotation_finalize() add the sentinel even when
> ann_count == 0, you should be able to safely use this code without
> needing to special case ann_count == 0 up above.  Which would be nice.

I think there's some confusion about when this code is used. To
confirm, it's only used when we're disassembling the program. If we're
disassembling the program, ann_count cannot be zero.

Does this make sense, or have I misunderstood something?

>> +
>> +   assume(ann != NULL);
>> +
>> +   ralloc_asprintf_rewrite_tail(&ann->error, &ann->error_length, error);
>
> Using asprintf with an ordinary (non-format) string is wrong...it'll
> interpret % characters...and you haven't given it any arguments.  You
> could do (..., "%s", error)...but this just seems like the wrong fit.

Oh, yeah. I had been using ralloc_vasprintf_rewrite_tail with vaargs
but then realized that I was just passing a single string in, so I
changed it to this shortly before sending.

>
> I would just do:
>
>    if (ann->error)
>       ralloc_strcat(&ann->error, error);
>    else
>       ann->error = ralloc_strdup(annotation->mem_ctx, error);
>
> You can drop the FIXME code below, as you'll be using the right memory
> context.  You can also drop the error_length field, as it's not needed.
>
> Sure, you gain extra strlen() calls when appending, but it's not worth
> optimizing...this is debug-only code...error strings aren't that
> long...and hopefully you don't have a cacophony of errors in the first
> place...
>
>> +
>> +   /* FIXME: ralloc_vasprintf_rewrite_tail() allocates memory out of the
>> +    * null context. We have to reparent the it if we want it to be freed
>> +    * with the rest of the annotation context.
>> +    */
>> +   ralloc_steal(annotation->mem_ctx, ann->error);
>> +}
>> diff --git a/src/mesa/drivers/dri/i965/intel_asm_annotation.h b/src/mesa/drivers/dri/i965/intel_asm_annotation.h
>> index 6c72326..662a4b4 100644
>> --- a/src/mesa/drivers/dri/i965/intel_asm_annotation.h
>> +++ b/src/mesa/drivers/dri/i965/intel_asm_annotation.h
>> @@ -37,6 +37,9 @@ struct cfg_t;
>>  struct annotation {
>>     int offset;
>>
>> +   size_t error_length;
>> +   char *error;
>> +
>>     /* Pointers to the basic block in the CFG if the instruction group starts
>>      * or ends a basic block.
>>      */
>> @@ -69,6 +72,10 @@ annotate(const struct brw_device_info *devinfo,
>>  void
>>  annotation_finalize(struct annotation_info *annotation, unsigned offset);
>>
>> +void
>> +annotation_insert_error(struct annotation_info *annotation, unsigned offset,
>> +                        const char *error);
>> +
>>  #ifdef __cplusplus
>>  } /* extern "C" */
>>  #endif
>>