[Mesa-dev] [PATCH 09/11] intel/tools: Refactor gen_disasm_disassemble() to use annotations

[Pohjolainen, Topi]

On Thu, May 04, 2017 at 11:27:57AM -0700, Matt Turner wrote:
> On Tue, May 2, 2017 at 6:03 AM, Iago Toral <itoral at igalia.com> wrote:
> > On Mon, 2017-05-01 at 13:54 -0700, Matt Turner wrote:
> >> Which will allow us to print validation errors found in shader
> >> assembly
> >> in GPU hang error states.
> >> ---
> >>  src/intel/tools/disasm.c | 71 +++++++++++++++++++++++++++++---------
> >> ----------
> >>  1 file changed, 43 insertions(+), 28 deletions(-)
> >>
> >> diff --git a/src/intel/tools/disasm.c b/src/intel/tools/disasm.c
> >> index 62256d2..361885b 100644
> >> --- a/src/intel/tools/disasm.c
> >> +++ b/src/intel/tools/disasm.c
> >> @@ -43,52 +43,67 @@ is_send(uint32_t opcode)
> >>             opcode == BRW_OPCODE_SENDSC );
> >>  }
> >>
> >> -void
> >> -gen_disasm_disassemble(struct gen_disasm *disasm, void *assembly,
> >> -                       int start, FILE *out)
> >> +static int
> >> +gen_disasm_find_end(struct gen_disasm *disasm, void *assembly, int
> >> start)
> >>  {
> >>     struct gen_device_info *devinfo = &disasm->devinfo;
> >> -   bool dump_hex = false;
> >>     int offset = start;
> >>
> >>     /* This loop exits when send-with-EOT or when opcode is 0 */
> >>     while (true) {
> >>        brw_inst *insn = assembly + offset;
> >> -      brw_inst uncompacted;
> >> -      bool compacted = brw_inst_cmpt_control(devinfo, insn);
> >> -      if (0)
> >> -         fprintf(out, "0x%08x: ", offset);
> >> -
> >> -      if (compacted) {
> >> -         brw_compact_inst *compacted = (void *)insn;
> >> -         if (dump_hex) {
> >> -            fprintf(out, "0x%08x 0x%08x                       ",
> >> -                   ((uint32_t *)insn)[1],
> >> -                   ((uint32_t *)insn)[0]);
> >> -         }
> >> -
> >> -         brw_uncompact_instruction(devinfo, &uncompacted,
> >> compacted);
> >> -         insn = &uncompacted;
> >> +
> >> +      if (brw_inst_cmpt_control(devinfo, insn)) {
> >>           offset += 8;
> >>        } else {
> >> -         if (dump_hex) {
> >> -            fprintf(out, "0x%08x 0x%08x 0x%08x 0x%08x ",
> >> -                   ((uint32_t *)insn)[3],
> >> -                   ((uint32_t *)insn)[2],
> >> -                   ((uint32_t *)insn)[1],
> >> -                   ((uint32_t *)insn)[0]);
> >> -         }
> >>           offset += 16;
> >>        }
> >>
> >> -      brw_disassemble_inst(out, devinfo, insn, compacted);
> >> -
> >>        /* Simplistic, but efficient way to terminate disasm */
> >>        uint32_t opcode = brw_inst_opcode(devinfo, insn);
> >>        if (opcode == 0 || (is_send(opcode) && brw_inst_eot(devinfo,
> >> insn))) {
> >>           break;
> >>        }
> >>     }
> >> +
> >> +   return offset;
> >> +}
> >> +
> >> +void
> >> +gen_disasm_disassemble(struct gen_disasm *disasm, void *assembly,
> >> +                       int start, FILE *out)
> >> +{
> >> +   struct gen_device_info *devinfo = &disasm->devinfo;
> >> +   int end = gen_disasm_find_end(disasm, assembly, start);
> >> +
> >> +   /* Make a dummy annotation structure that
> >> brw_validate_instructions
> >> +    * can work from.
> >> +    */
> >> +   struct annotation_info annotation_info = {
> >> +      .ann_count = 1,
> >> +      .ann_size = 2,
> >> +   };
> >> +   annotation_info.mem_ctx = ralloc_context(NULL);
> >> +   annotation_info.ann = rzalloc_array(annotation_info.mem_ctx,
> >> +                                       struct annotation,
> >> +                                       annotation_info.ann_size);
> >> +   annotation_info.ann[0].offset = start;
> >> +   annotation_info.ann[1].offset = end;
> >> +   brw_validate_instructions(devinfo, assembly, start, end,
> >> &annotation_info);
> >> +   struct annotation *annotation = annotation_info.ann;
> >> +
> >> +   for (int i = 0; i < annotation_info.ann_count; i++) {
> >> +      int start_offset = annotation[i].offset;
> >> +      int end_offset = annotation[i + 1].offset;
> >
> > I was going to say that this code seems to overflow when i
> > == annotation_info.ann_count - 1, but then I noticed that there are
> > similar loops in other parts of the code and that you initialized
> > ann_count to just 1 even though you have two initial annotations, so I
> > guess this is how this is supposed to work, right?
> 
> Yeah, exactly. I'm not sure it's a good way to handle it though. To be
> honest, I had a hard time remembering how it was supposed to work and
> I wrote the annotation system.
> 
> I think a linked list would be a lot more natural than the array/count.

Sounds like it. While not ideal I think we can live with this for now:

Reviewed-by: Topi Pohjolainen <topi.pohjolainen at intel.com>