[Mesa-dev] [PATCH 6/9] i965: Add annotation_insert_error() and support for printing errors.
Kenneth Graunke
kenneth at whitecape.org
Tue Nov 3 22:44:43 PST 2015
On Tuesday, November 03, 2015 10:20:26 PM Matt Turner wrote:
> 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?
>
I see, we add annotation entries even if INTEL_DEBUG=ann is unset...we
just leave them with no IR/string contents, so they don't show up.
Okay, then ann_count is definitely non-zero. Nevermind this.
> > 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.
Eh? Doesn't the annotation with offset = 8 include the instruction at
offset 8?
> > 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 i'm comparing against insertion_point, which is 1 instruction
beyond offset, so you skipping <= offset is compatible with me skipping
on < insertion_point and handling == insertion_point.
> >
> > /* 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 :)
My mistake, we can drop that line.
> 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.
Okay, that makes sense.
>
> > 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;
> > }
> > }
I still think my code is correct and easier to read...
> >
> > 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?
I missed that we actually add annotation structures when running without
INTEL_DEBUG=ann. I figured if we compiled a shader and nobody bothered
to add annotations, this would be 0. I was wrong.
>
> >> +
> >> + 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
> >>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20151103/a97e341d/attachment-0001.sig>
More information about the mesa-dev
mailing list