[Mesa-dev] [PATCH 3/4] i965: Rewrite disassembly annotation code

[Matt Turner]

On Fri, Nov 17, 2017 at 11:16 AM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On Thursday, November 16, 2017 8:47:33 PM PST Matt Turner wrote:
>> diff --git a/src/intel/compiler/intel_asm_annotation.c b/src/intel/compiler/intel_asm_annotation.c
>> index 26ab4b9818..fa37f248d1 100644
>> --- a/src/intel/compiler/intel_asm_annotation.c
>> +++ b/src/intel/compiler/intel_asm_annotation.c
>> -void annotate(const struct gen_device_info *devinfo,
>> -              struct annotation_info *annotation, const struct cfg_t *cfg,
>> -              struct backend_instruction *inst, unsigned offset)
>> +struct inst_group *
>> +disasm_new_inst_group(struct disasm_info *disasm, unsigned next_inst_offset)
>>  {
>> -   if (annotation->mem_ctx == NULL)
>> -      annotation->mem_ctx = ralloc_context(NULL);
>> +   struct inst_group *tail =
>> +      rzalloc(disasm->mem_ctx, struct inst_group);
>> +   tail->offset = next_inst_offset;
>> +   exec_list_push_tail(disasm->group_list, &tail->link);
>> +   return tail;
>> +}
>>
>> -   if (!annotation_array_ensure_space(annotation))
>> -      return;
>> +void
>> +disasm_annotate(struct disasm_info *disasm,
>> +                struct backend_instruction *inst, unsigned offset)
>> +{
>> +   const struct gen_device_info *devinfo = disasm->devinfo;
>> +   const struct cfg_t *cfg = disasm->cfg;
>> +
>> +   struct inst_group *group;
>> +   if (!disasm->use_tail) {
>> +      group = disasm_new_inst_group(disasm, offset);
>> +      disasm->use_tail = false;
>
> Err...it's already false in this case.  You just checked it two lines
> earlier, and disasm_new_inst_group doesn't change.
>
>> +   } else {
>> +      group = exec_node_data(struct inst_group,
>> +                             exec_list_get_tail_raw(disasm->group_list), link);
>
> I suspect you meant to set disasm->use_tail = false here instead.

Oops. Yes, you're exactly right.

>
>> +   }
>
> With that fixed, the series is:
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
>
> I also think you could simply embed the exec_list in the disasm_info
> structure rather than heap allocating it.  It doesn't seem to buy you
> anything.  But, I'll leave that up to you.

That's a great suggestion. Thanks.