2024-05-07 09:51:58

by Christophe Leroy

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



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 <[email protected]>
> ---
> 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;