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

[Matt Turner]

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.