Re: [PATCH 2/3] tools/erf/util/annotate: Set register_char and memory_ref_char for powerpc
All instructions with a primary opcode from 32 to 63 are memory insns,
and no others. It's trivial to see whether it is a load or store, too
(just bit 3 of the insn). Trying to parse disassembled code is much
harder, and you easily make some mistakes here.
On Sat, Mar 09, 2024 at 12:55:12PM +0530, Athira Rajeev wrote:
> To identify if the instruction has any memory reference,
> "memory_ref_char" field needs to be set for specific architecture.
> Example memory instruction:
> lwz r10,0(r9)
>
> Here "(" is the memory_ref_char. Set this as part of arch->objdump
What about "lwzx r10,0,r19", semantically exactly the same instruction?
There is no paren in there. Not all instructions using parens are
memory insns, either, not in assembler code at least.
> To get register number and access offset from the given instruction,
> arch->objdump.register_char is used. In case of powerpc, the register
> char is "r" (since reg names are r0 to r31). Hence set register_char
> as "r".
cr0..cr7
r0..r31
f0..f31
v0..v31
vs0..vs63
and many other spellings. Plain "0..63" is also fine.
The "0" in my lwzx example is a register field as well (and it stands
for "no register", not for "r0"). Called "(RA|0)" usually (incidentally,
see the parens there as well, oh joy).
Don't you have the binary code here as well, not just a disassembled
representation of it? It is way easier to just use that, and you'll get
much better results that way :-)
Segher
Re: [PATCH 01/19] vdso: Consolidate vdso_calc_delta()
On Fri, Mar 8, 2024 at 5:15 AM Adrian Hunter wrote:
>
> Consolidate vdso_calc_delta(), in preparation for further simplification.
>
> Suggested-by: Thomas Gleixner
> Signed-off-by: Adrian Hunter
> ---
> arch/powerpc/include/asm/vdso/gettimeofday.h | 17 ++---
> arch/s390/include/asm/vdso/gettimeofday.h| 7 ++-
> lib/vdso/gettimeofday.c | 4
> 3 files changed, 8 insertions(+), 20 deletions(-)
>
> --- a/lib/vdso/gettimeofday.c
> +++ b/lib/vdso/gettimeofday.c
> @@ -13,7 +13,11 @@
> static __always_inline
> u64 vdso_calc_delta(u64 cycles, u64 last, u64 mask, u32 mult)
> {
> +#ifdef VDSO_DELTA_NOMASK
> + return (cycles - last) * mult;
> +#else
> return ((cycles - last) & mask) * mult;
> +#endif
> }
Nit: Just for readability, usually we avoid #ifdefs inside of functions.
Instead maybe:
#ifdef VDSO_DELTA_NOMASK
static __always_inline
u64 vdso_calc_delta(u64 cycles, u64 last, u64 mask, u32 mult)
{
return (cycles - last) * mult;
}
#else
static __always_inline
u64 vdso_calc_delta(u64 cycles, u64 last, u64 mask, u32 mult)
{
return ((cycles - last) & mask) * mult;
}
#endif
Re: [PATCH 3/3] tools/perf/arch/powerc: Add get_arch_regnum for powerpc
Le 09/03/2024 à 08:25, Athira Rajeev a écrit :
> The function get_dwarf_regnum() returns a DWARF register number
> from a register name string. This calls arch specific function
> get_arch_regnum to return register number for corresponding arch.
> Add mappings for register name to register number in powerpc code:
> arch/powerpc/util/dwarf-regs.c
>
> Signed-off-by: Athira Rajeev
> ---
> tools/perf/arch/powerpc/util/dwarf-regs.c | 29 +++
> 1 file changed, 29 insertions(+)
>
> diff --git a/tools/perf/arch/powerpc/util/dwarf-regs.c
> b/tools/perf/arch/powerpc/util/dwarf-regs.c
> index 0c4f4caf53ac..d955e3e577ea 100644
> --- a/tools/perf/arch/powerpc/util/dwarf-regs.c
> +++ b/tools/perf/arch/powerpc/util/dwarf-regs.c
> @@ -98,3 +98,32 @@ int regs_query_register_offset(const char *name)
> return roff->ptregs_offset;
> return -EINVAL;
> }
> +
> +struct dwarf_regs_idx {
> + const char *name;
> + int idx;
> +};
> +
> +static const struct dwarf_regs_idx powerpc_regidx_table[] = {
> + { "r0", 0 }, { "r1", 1 }, { "r2", 2 }, { "r3", 3 }, { "r4", 4 },
> + { "r5", 5 }, { "r6", 6 }, { "r7", 7 }, { "r8", 8 }, { "r9", 9 },
> + { "r10", 10 }, { "r11", 11 }, { "r12", 12 }, { "r13", 13 }, { "r14", 14
> },
> + { "r15", 15 }, { "r16", 16 }, { "r17", 17 }, { "r18", 18 }, { "r19", 19
> },
> + { "r20", 20 }, { "r21", 21 }, { "r22", 22 }, { "r23", 23 }, { "r24", 24
> },
> + { "r25", 25 }, { "r26", 26 }, { "r27", 27 }, { "r27", 27 }, { "r28", 28
> },
> + { "r29", 29 }, { "r30", 30 }, { "r31", 31 },
> +};
> +
> +int get_arch_regnum(const char *name)
> +{
> + unsigned int i;
> +
> + if (*name != 'r')
> + return -EINVAL;
> +
> + for (i = 0; i < ARRAY_SIZE(powerpc_regidx_table); i++)
> + if (!strcmp(powerpc_regidx_table[i].name, name))
> + return powerpc_regidx_table[i].idx;
Can you do more simple ?
Something like:
int n;
if (*name != 'r')
return -EINVAL;
n = atoi(name + 1);
return n >= 0 && n < 32 ? n : -ENOENT;
> +
> + return -ENOENT;
> +}
Re: [PATCH 1/3] tools/perf/arch/powerpc: Add load/store in powerpc annotate instructions for data type profling
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
> ---
> .../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
>
> +/*
> + * 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 = '#';
