On Sun, Jul 07, 2024 at 08:14:19PM +0530, Athira Rajeev wrote: > Since the "ins.name" is not set while using raw instruction, > perf annotate with insn-stat gives wrong data: > > Result from "./perf annotate --data-type --insn-stat": > > Annotate Instruction stats > total 615, ok 419 (68.1%), bad 196 (31.9%) > > Name : Good Bad > ----------------------------------------------------------- > : 419 196 > > Patch sets "dl->ins.name" in arch specific function "check_ppc_insn" > while initialising "struct disasm_line". Also update "ins_find" function > to pass "struct disasm_line" as a parameter so as to set its name field > in arch specific call. > > With the patch changes: > > Annotate Instruction stats > total 609, ok 446 (73.2%), bad 163 (26.8%) > > Name/opcode: Good Bad > ----------------------------------------------------------- > 58 : 323 80 > 32 : 49 43 > 34 : 33 11 > OP_31_XOP_LDX : 8 20 > 40 : 23 0 > OP_31_XOP_LWARX : 5 1 > OP_31_XOP_LWZX : 2 3 > OP_31_XOP_LDARX : 3 0 > 33 : 0 2 > OP_31_XOP_LBZX : 0 1 > OP_31_XOP_LWAX : 0 1 > OP_31_XOP_LHZX : 0 1 > > Signed-off-by: Athira Rajeev <atraj...@linux.vnet.ibm.com> > --- > .../perf/arch/powerpc/annotate/instructions.c | 18 +++++++++++++++--- > tools/perf/builtin-annotate.c | 4 ++-- > tools/perf/util/annotate.c | 2 +- > tools/perf/util/disasm.c | 10 +++++----- > tools/perf/util/disasm.h | 2 +- > 5 files changed, 24 insertions(+), 12 deletions(-) > > diff --git a/tools/perf/arch/powerpc/annotate/instructions.c > b/tools/perf/arch/powerpc/annotate/instructions.c > index af1032572bf3..ede9eeade0ab 100644 > --- a/tools/perf/arch/powerpc/annotate/instructions.c > +++ b/tools/perf/arch/powerpc/annotate/instructions.c > @@ -189,8 +189,9 @@ static int cmp_offset(const void *a, const void *b) > return (val1->value - val2->value); > } > > -static struct ins_ops *check_ppc_insn(u32 raw_insn) > +static struct ins_ops *check_ppc_insn(struct disasm_line *dl) > { > + int raw_insn = dl->raw.raw_insn; > int opcode = PPC_OP(raw_insn); > int mem_insn_31 = PPC_21_30(raw_insn); > struct insn_offset *ret; > @@ -198,19 +199,30 @@ static struct ins_ops *check_ppc_insn(u32 raw_insn) > "OP_31_INSN", > mem_insn_31 > }; > + char name_insn[32]; > > /* > * Instructions with opcode 32 to 63 are memory > * instructions in powerpc > */ > if ((opcode & 0x20)) { > + /* > + * Set name in case of raw instruction to > + * opcode to be used in insn-stat > + */ > + if (!strlen(dl->ins.name)) { > + sprintf(name_insn, "%d", opcode); > + dl->ins.name = strdup(name_insn); > + } > return &load_store_ops; > } else if (opcode == 31) { > /* Check for memory instructions with opcode 31 */ > ret = bsearch(&mem_insns_31_opcode, ins_array, > ARRAY_SIZE(ins_array), sizeof(ins_array[0]), cmp_offset); > - if (ret != NULL) > + if (ret) { > + if (!strlen(dl->ins.name)) > + dl->ins.name = strdup(ret->name); > return &load_store_ops; > - else { > + } else { > mem_insns_31_opcode.value = PPC_22_30(raw_insn); > ret = bsearch(&mem_insns_31_opcode, > arithmetic_ins_op_31, ARRAY_SIZE(arithmetic_ins_op_31), > sizeof(arithmetic_ins_op_31[0]), > cmp_offset); > diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c > index b10b7f005658..68e929d4746e 100644 > --- a/tools/perf/builtin-annotate.c > +++ b/tools/perf/builtin-annotate.c > @@ -396,10 +396,10 @@ static void print_annotate_item_stat(struct list_head > *head, const char *title) > printf("total %d, ok %d (%.1f%%), bad %d (%.1f%%)\n\n", total, > total_good, 100.0 * total_good / (total ?: 1), > total_bad, 100.0 * total_bad / (total ?: 1)); > - printf(" %-10s: %5s %5s\n", "Name", "Good", "Bad"); > + printf(" %-10s: %5s %5s\n", "Name/opcode", "Good", "Bad");
It should be "%-20s". Thanks, Namhyung > printf("-----------------------------------------------------------\n"); > list_for_each_entry(istat, head, list) > - printf(" %-10s: %5d %5d\n", istat->name, istat->good, > istat->bad); > + printf(" %-20s: %5d %5d\n", istat->name, istat->good, > istat->bad); > printf("\n"); > } > > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c > index 8db2f32700aa..e1f24dff8042 100644 > --- a/tools/perf/util/annotate.c > +++ b/tools/perf/util/annotate.c > @@ -2229,7 +2229,7 @@ static struct annotated_item_stat > *annotate_data_stat(struct list_head *head, > return NULL; > > istat->name = strdup(name); > - if (istat->name == NULL) { > + if ((istat->name == NULL) || (!strlen(istat->name))) { > free(istat); > return NULL; > } > diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c > index 63681df6482b..cd283c42195c 100644 > --- a/tools/perf/util/disasm.c > +++ b/tools/perf/util/disasm.c > @@ -859,7 +859,7 @@ static void ins__sort(struct arch *arch) > qsort(arch->instructions, nmemb, sizeof(struct ins), ins__cmp); > } > > -static struct ins_ops *__ins__find(struct arch *arch, const char *name, u32 > raw_insn) > +static struct ins_ops *__ins__find(struct arch *arch, const char *name, > struct disasm_line *dl) > { > struct ins *ins; > const int nmemb = arch->nr_instructions; > @@ -871,7 +871,7 @@ static struct ins_ops *__ins__find(struct arch *arch, > const char *name, u32 raw_ > */ > struct ins_ops *ops; > > - ops = check_ppc_insn(raw_insn); > + ops = check_ppc_insn(dl); > if (ops) > return ops; > } > @@ -905,9 +905,9 @@ static struct ins_ops *__ins__find(struct arch *arch, > const char *name, u32 raw_ > return ins ? ins->ops : NULL; > } > > -struct ins_ops *ins__find(struct arch *arch, const char *name, u32 raw_insn) > +struct ins_ops *ins__find(struct arch *arch, const char *name, struct > disasm_line *dl) > { > - struct ins_ops *ops = __ins__find(arch, name, raw_insn); > + struct ins_ops *ops = __ins__find(arch, name, dl); > > if (!ops && arch->associate_instruction_ops) > ops = arch->associate_instruction_ops(arch, name); > @@ -917,7 +917,7 @@ struct ins_ops *ins__find(struct arch *arch, const char > *name, u32 raw_insn) > > static void disasm_line__init_ins(struct disasm_line *dl, struct arch *arch, > struct map_symbol *ms) > { > - dl->ins.ops = ins__find(arch, dl->ins.name, dl->raw.raw_insn); > + dl->ins.ops = ins__find(arch, dl->ins.name, dl); > > if (!dl->ins.ops) > return; > diff --git a/tools/perf/util/disasm.h b/tools/perf/util/disasm.h > index c1bb1e484bfb..f56beedeb9da 100644 > --- a/tools/perf/util/disasm.h > +++ b/tools/perf/util/disasm.h > @@ -105,7 +105,7 @@ struct annotate_args { > struct arch *arch__find(const char *name); > bool arch__is(struct arch *arch, const char *name); > > -struct ins_ops *ins__find(struct arch *arch, const char *name, u32 raw_insn); > +struct ins_ops *ins__find(struct arch *arch, const char *name, struct > disasm_line *dl); > int ins__scnprintf(struct ins *ins, char *bf, size_t size, > struct ins_operands *ops, int max_ins_name); > > -- > 2.43.0 >