Re: [PATCH 1/3] tools/perf/arch/powerpc: Add load/store in powerpc annotate instructions for data type profling

[Athira Rajeev <atrajeev@linux.vnet.ibm.com>]

> On 09-Mar-2024, at 3:18 PM, Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
> 
> 
> 
> Le 09/03/2024 à 08:25, Athira Rajeev a écrit :
>> Add powerpc instruction nmemonic table to associate load/store
>> instructions with move_ops. mov_ops is used to identify mem_type
>> to associate instruction with data type and offset. Also initialize
>> and allocate arch specific fields for nr_instructions, instructions and
>> nr_instructions_allocate.
>> 
>> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
>> ---
>>  .../perf/arch/powerpc/annotate/instructions.c | 66 +++++++++++++++++++
>>  1 file changed, 66 insertions(+)
>> 
>> diff --git a/tools/perf/arch/powerpc/annotate/instructions.c b/tools/perf/arch/powerpc/annotate/instructions.c
>> index a3f423c27cae..07af4442be38 100644
>> --- a/tools/perf/arch/powerpc/annotate/instructions.c
>> +++ b/tools/perf/arch/powerpc/annotate/instructions.c
>> @@ -1,6 +1,65 @@
>>  // SPDX-License-Identifier: GPL-2.0
>>  #include <linux/compiler.h>
>> 
>> +/*
>> + * powerpc instruction nmemonic table to associate load/store instructions with
>> + * move_ops. mov_ops is used to identify mem_type to associate instruction with
>> + * data type and offset.
>> + */
>> +static struct ins powerpc__instructions[] = {
>> + { .name = "lbz", .ops = &mov_ops,  },
>> + { .name = "lbzx", .ops = &mov_ops,  },
>> + { .name = "lbzu", .ops = &mov_ops,  },
>> + { .name = "lbzux", .ops = &mov_ops,  },
>> + { .name = "lhz", .ops = &mov_ops,  },
>> + { .name = "lhzx", .ops = &mov_ops,  },
>> + { .name = "lhzu", .ops = &mov_ops,  },
>> + { .name = "lhzux", .ops = &mov_ops,  },
>> + { .name = "lha", .ops = &mov_ops,  },
>> + { .name = "lhax", .ops = &mov_ops,  },
>> + { .name = "lhau", .ops = &mov_ops,  },
>> + { .name = "lhaux", .ops = &mov_ops,  },
>> + { .name = "lwz", .ops = &mov_ops,  },
>> + { .name = "lwzx", .ops = &mov_ops,  },
>> + { .name = "lwzu", .ops = &mov_ops,  },
>> + { .name = "lwzux", .ops = &mov_ops,  },
>> + { .name = "lwa", .ops = &mov_ops,  },
>> + { .name = "lwax", .ops = &mov_ops,  },
>> + { .name = "lwaux", .ops = &mov_ops,  },
>> + { .name = "ld", .ops = &mov_ops,  },
>> + { .name = "ldx", .ops = &mov_ops,  },
>> + { .name = "ldu", .ops = &mov_ops,  },
>> + { .name = "ldux", .ops = &mov_ops,  },
>> + { .name = "stb", .ops = &mov_ops,  },
>> + { .name = "stbx", .ops = &mov_ops,  },
>> + { .name = "stbu", .ops = &mov_ops,  },
>> + { .name = "stbux", .ops = &mov_ops,  },
>> + { .name = "sth", .ops = &mov_ops,  },
>> + { .name = "sthx", .ops = &mov_ops,  },
>> + { .name = "sthu", .ops = &mov_ops,  },
>> + { .name = "sthux", .ops = &mov_ops,  },
>> + { .name = "stw", .ops = &mov_ops,  },
>> + { .name = "stwx", .ops = &mov_ops,  },
>> + { .name = "stwu", .ops = &mov_ops,  },
>> + { .name = "stwux", .ops = &mov_ops,  },
>> + { .name = "std", .ops = &mov_ops,  },
>> + { .name = "stdx", .ops = &mov_ops,  },
>> + { .name = "stdu", .ops = &mov_ops,  },
>> + { .name = "stdux", .ops = &mov_ops,  },
>> + { .name = "lhbrx", .ops = &mov_ops,  },
>> + { .name = "sthbrx", .ops = &mov_ops,  },
>> + { .name = "lwbrx", .ops = &mov_ops,  },
>> + { .name = "stwbrx", .ops = &mov_ops,  },
>> + { .name = "ldbrx", .ops = &mov_ops,  },
>> + { .name = "stdbrx", .ops = &mov_ops,  },
>> + { .name = "lmw", .ops = &mov_ops,  },
>> + { .name = "stmw", .ops = &mov_ops,  },
>> + { .name = "lswi", .ops = &mov_ops,  },
>> + { .name = "lswx", .ops = &mov_ops,  },
>> + { .name = "stswi", .ops = &mov_ops,  },
>> + { .name = "stswx", .ops = &mov_ops,  },
>> +};
> 
> What about lwarx and stwcx ?
Yes, Will add those in next version

> 
>> +
>>  static struct ins_ops *powerpc__associate_instruction_ops(struct arch *arch, const char *name)
>>  {
>>   int i;
>> @@ -52,6 +111,13 @@ static struct ins_ops *powerpc__associate_instruction_ops(struct arch *arch, con
>>  static int powerpc__annotate_init(struct arch *arch, char *cpuid __maybe_unused)
>>  {
>>   if (!arch->initialized) {
>> + arch->nr_instructions = ARRAY_SIZE(powerpc__instructions);
>> + arch->instructions = calloc(arch->nr_instructions, sizeof(struct ins));
>> + if (arch->instructions == NULL)
> 
> Prefered form is
> 
> if (!arch->instructions)
Ok , will make this change

> 
>> + return -ENOMEM;
>> +
>> + memcpy(arch->instructions, (struct ins *)powerpc__instructions, sizeof(struct ins) * arch->nr_instructions);
> 
> No need to cast powerpc__instructions, it is already a pointer.
Yes, I will correct it


Thanks
Athira Rajeev
> 
> 
>> + arch->nr_instructions_allocated = arch->nr_instructions;
>>   arch->initialized = true;
>>   arch->associate_instruction_ops = powerpc__associate_instruction_ops;
>>   arch->objdump.comment_char      = '#';