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

[Christophe Leroy <christophe.leroy@csgroup.eu>]

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 ?

> +
>   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)

> +			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.


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