Re: [PATCH V2 5/9] tools/perf: Update parameters for reg extract functions to use raw instruction on powerpc

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

Le 06/05/2024 à 14:19, Athira Rajeev a écrit :
> Use the raw instruction code and macros to identify memory instructions,
> extract register fields and also offset. The implementation addresses
> the D-form, X-form, DS-form instructions. Two main functions are added.
> New parse function "load_store__parse" as instruction ops parser for
> memory instructions. Unlink other parser (like mov__parse), this parser
> fills in only the "raw" field for source/target and new added "mem_ref"
> field. This is because, here there is no need to parse the disassembled
> code and arch specific macros will take care of extracting offset and
> regs which is easier and will be precise.
> 
> In powerpc, all instructions with a primary opcode from 32 to 63
> are memory instructions. Update "ins__find" function to have "opcode"
> also as a parameter. Don't use the "extract_reg_offset", instead use
> newly added function "get_arch_regs" which will set these fields: reg1,
> reg2, offset depending of where it is source or target ops.

Yes all instructions with a primary opcode from 32 to 63 are memory 
instructions, but not all memory instructions have opcode 32 to 63.

> 
> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> ---
>   tools/perf/arch/powerpc/util/dwarf-regs.c | 33 +++++++++++++
>   tools/perf/util/annotate.c                | 22 ++++++++-
>   tools/perf/util/disasm.c                  | 59 +++++++++++++++++++++--
>   tools/perf/util/disasm.h                  |  4 +-
>   tools/perf/util/include/dwarf-regs.h      |  4 +-
>   5 files changed, 114 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/perf/arch/powerpc/util/dwarf-regs.c b/tools/perf/arch/powerpc/util/dwarf-regs.c
> index e60a71fd846e..3121c70dc0d3 100644
> --- a/tools/perf/arch/powerpc/util/dwarf-regs.c
> +++ b/tools/perf/arch/powerpc/util/dwarf-regs.c
> @@ -102,6 +102,9 @@ int regs_query_register_offset(const char *name)
>   #define	PPC_OP(op)	(((op) >> 26) & 0x3F)
>   #define PPC_RA(a)	(((a) >> 16) & 0x1f)
>   #define PPC_RT(t)	(((t) >> 21) & 0x1f)
> +#define PPC_RB(b)	(((b) >> 11) & 0x1f)
> +#define PPC_D(D)	((D) & 0xfffe)
> +#define PPC_DS(DS)	((DS) & 0xfffc)
>   
>   int get_opcode_insn(unsigned int raw_insn)
>   {
> @@ -117,3 +120,33 @@ int get_target_reg(unsigned int raw_insn)
>   {
>   	return PPC_RT(raw_insn);
>   }
> +
> +int get_offset_opcode(int raw_insn __maybe_unused)
> +{
> +	int opcode = PPC_OP(raw_insn);
> +
> +	/* DS- form */
> +	if ((opcode == 58) || (opcode == 62))

Can you re-use macros from arch/powerpc/include/asm/ppc-opcode.h ?

#define OP_LD		58
#define OP_STD		62


> +		return PPC_DS(raw_insn);
> +	else
> +		return PPC_D(raw_insn);
> +}
> +
> +/*
> + * Fills the required fields for op_loc depending on if it
> + * is a source of target.
> + * D form: ins RT,D(RA) -> src_reg1 = RA, offset = D, dst_reg1 = RT
> + * DS form: ins RT,DS(RA) -> src_reg1 = RA, offset = DS, dst_reg1 = RT
> + * X form: ins RT,RA,RB -> src_reg1 = RA, src_reg2 = RB, dst_reg1 = RT
> + */
> +void get_arch_regs(int raw_insn __maybe_unused, int is_source __maybe_unused, struct annotated_op_loc *op_loc __maybe_unused)
> +{
> +	if (is_source)
> +		op_loc->reg1 = get_source_reg(raw_insn);
> +	else
> +		op_loc->reg1 = get_target_reg(raw_insn);
> +	if (op_loc->multi_regs)
> +		op_loc->reg2 = PPC_RB(raw_insn);
> +	if (op_loc->mem_ref)
> +		op_loc->offset = get_offset_opcode(raw_insn);
> +}

> diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
> index 85692f73e78f..f41a0fadeab4 100644
> --- a/tools/perf/util/disasm.c
> +++ b/tools/perf/util/disasm.c
> @@ -760,11 +800,20 @@ 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)
> +static struct ins_ops *__ins__find(struct arch *arch, const char *name, int opcode)
>   {
>   	struct ins *ins;
>   	const int nmemb = arch->nr_instructions;
>   
> +	if (arch__is(arch, "powerpc")) {
> +		/*
> +		 * Instructions with a primary opcode from 32 to 63
> +		 * are memory instructions in powerpc.
> +		 */
> +		if ((opcode >= 31) && (opcode <= 63))

Could just be if ((opcode & 0x20)) I guess.

By the way your test is wrong because opcode 31 is not only memory 
instructions, see example below (not exhaustive):

#define OP_31_XOP_TRAP      4		==> No
#define OP_31_XOP_LDX       21		==> Yes
#define OP_31_XOP_LWZX      23		==> Yes
#define OP_31_XOP_LDUX      53		==> Yes
#define OP_31_XOP_DCBST     54		==> No
#define OP_31_XOP_LWZUX     55		==> Yes
#define OP_31_XOP_TRAP_64   68		==> No
#define OP_31_XOP_DCBF      86		==> No
#define OP_31_XOP_LBZX      87		==> Yes
#define OP_31_XOP_STDX      149		==> Yes
#define OP_31_XOP_STWX      151		==> Yes




> +			return &load_store_ops;
> +	}
> +
>   	if (!arch->sorted_instructions) {
>   		ins__sort(arch);
>   		arch->sorted_instructions = true;