On Tue, Nov 3, 2015 at 10:44 PM, Kenneth Graunke <kenn...@whitecape.org> wrote:
> On Tuesday, November 03, 2015 10:20:26 PM Matt Turner wrote:
>> On Tue, Nov 3, 2015 at 9:47 PM, Kenneth Graunke <kenn...@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?
Yes.
Basically, the loop index is off-by-one, as seen by the <= (instead of
<) and the messed up cur/next pointers.
>> > 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.
Yep, makes sense.
>> >
>> > /* 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...
Given that I had to think so much about how mine works, I think I agree. :)
>> >
>> > 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.
Right... the names are sort of confusing. I added this when IR
annotations were still very much a thing, so I named it after that,
but it's definitely used any time we're printing the disassembly, even
without INTEL_DEBUG=ann.
I'd be happy to rename stuff.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
