Re: [PATCH V2 7/9] tools/perf: Update instruction tracking with add instruction
> On 7 May 2024, at 3:28 PM, Christophe Leroy
> wrote:
>
>
>
> Le 06/05/2024 à 14:19, Athira Rajeev a écrit :
>> Update instruction tracking with add instruction. Apart from "mr"
>> instruction, the register state is carried on by other insns, ie,
>> "add, addi, addis". Since these are not memory instructions and doesn't
>> fall in the range of (32 to 63), add these as part of nmemonic table.
>> For now, add* instructions are added. There is possibility of getting
>> more added here. But to extract regs, still the binary code will be
>> used. So associate this with "load_store_ops" itself and no other
>> changes required.
>
> Looks fragile.
>
> How do you handle addc, adde, addic ?
> And also addme, addze, which only have two operands instead of 3 ?
Hi Christophe,
Thanks for pointing these cases. Will address these cases in next version
Thanks
Athira
>
>>
>> Signed-off-by: Athira Rajeev
>> ---
>> .../perf/arch/powerpc/annotate/instructions.c | 21 +++
>> tools/perf/util/disasm.c | 1 +
>> 2 files changed, 22 insertions(+)
>>
>> diff --git a/tools/perf/arch/powerpc/annotate/instructions.c
>> b/tools/perf/arch/powerpc/annotate/instructions.c
>> index cce7023951fe..1f35d8a65bb4 100644
>> --- a/tools/perf/arch/powerpc/annotate/instructions.c
>> +++ b/tools/perf/arch/powerpc/annotate/instructions.c
>> @@ -1,6 +1,17 @@
>> // 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 add/mr to do instruction tracking.
>> + */
>> +static struct ins powerpc__instructions[] = {
>> + { .name = "mr", .ops = _store_ops, },
>> + { .name = "addi", .ops = _store_ops, },
>> + { .name = "addis", .ops = _store_ops, },
>> + { .name = "add",.ops = _store_ops, },
>> +};
>> +
>> static struct ins_ops *powerpc__associate_instruction_ops(struct arch
>> *arch, const char *name)
>> {
>> int i;
>> @@ -75,6 +86,9 @@ static void update_insn_state_powerpc(struct type_state
>> *state,
>> if (annotate_get_insn_location(dloc->arch, dl, ) < 0)
>> return;
>>
>> + if (!strncmp(dl->ins.name, "add", 3))
>> + goto regs_check;
>> +
>> if (strncmp(dl->ins.name, "mr", 2))
>> return;
>>
>> @@ -85,6 +99,7 @@ static void update_insn_state_powerpc(struct type_state
>> *state,
>> dst->reg1 = src_reg;
>> }
>>
>> +regs_check:
>> if (!has_reg_type(state, dst->reg1))
>> return;
>>
>> @@ -115,6 +130,12 @@ static void update_insn_state_powerpc(struct type_state
>> *state __maybe_unused, s
>> 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)
>> + return -ENOMEM;
>> + memcpy(arch->instructions, powerpc__instructions, sizeof(struct ins) *
>> arch->nr_instructions);
>> + arch->nr_instructions_allocated = arch->nr_instructions;
>> arch->initialized = true;
>> arch->associate_instruction_ops = powerpc__associate_instruction_ops;
>> arch->objdump.comment_char = '#';
>> diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
>> index ac6b8b8da38a..32cf506a9010 100644
>> --- a/tools/perf/util/disasm.c
>> +++ b/tools/perf/util/disasm.c
>> @@ -36,6 +36,7 @@ static struct ins_ops mov_ops;
>> static struct ins_ops nop_ops;
>> static struct ins_ops lock_ops;
>> static struct ins_ops ret_ops;
>> +static struct ins_ops load_store_ops;
>>
>> static int jump__scnprintf(struct ins *ins, char *bf, size_t size,
>> struct ins_operands *ops, int max_ins_name);
Re: [PATCH V2 7/9] tools/perf: Update instruction tracking with add instruction
Le 06/05/2024 à 14:19, Athira Rajeev a écrit :
> Update instruction tracking with add instruction. Apart from "mr"
> instruction, the register state is carried on by other insns, ie,
> "add, addi, addis". Since these are not memory instructions and doesn't
> fall in the range of (32 to 63), add these as part of nmemonic table.
> For now, add* instructions are added. There is possibility of getting
> more added here. But to extract regs, still the binary code will be
> used. So associate this with "load_store_ops" itself and no other
> changes required.
Looks fragile.
How do you handle addc, adde, addic ?
And also addme, addze, which only have two operands instead of 3 ?
>
> Signed-off-by: Athira Rajeev
> ---
> .../perf/arch/powerpc/annotate/instructions.c | 21 +++
> tools/perf/util/disasm.c | 1 +
> 2 files changed, 22 insertions(+)
>
> diff --git a/tools/perf/arch/powerpc/annotate/instructions.c
> b/tools/perf/arch/powerpc/annotate/instructions.c
> index cce7023951fe..1f35d8a65bb4 100644
> --- a/tools/perf/arch/powerpc/annotate/instructions.c
> +++ b/tools/perf/arch/powerpc/annotate/instructions.c
> @@ -1,6 +1,17 @@
> // 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 add/mr to do instruction tracking.
> + */
> +static struct ins powerpc__instructions[] = {
> + { .name = "mr", .ops = _store_ops, },
> + { .name = "addi", .ops = _store_ops, },
> + { .name = "addis", .ops = _store_ops, },
> + { .name = "add",.ops = _store_ops, },
> +};
> +
> static struct ins_ops *powerpc__associate_instruction_ops(struct arch
> *arch, const char *name)
> {
> int i;
> @@ -75,6 +86,9 @@ static void update_insn_state_powerpc(struct type_state
> *state,
> if (annotate_get_insn_location(dloc->arch, dl, ) < 0)
> return;
>
> + if (!strncmp(dl->ins.name, "add", 3))
> + goto regs_check;
> +
> if (strncmp(dl->ins.name, "mr", 2))
> return;
>
> @@ -85,6 +99,7 @@ static void update_insn_state_powerpc(struct type_state
> *state,
> dst->reg1 = src_reg;
> }
>
> +regs_check:
> if (!has_reg_type(state, dst->reg1))
> return;
>
> @@ -115,6 +130,12 @@ static void update_insn_state_powerpc(struct type_state
> *state __maybe_unused, s
> 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)
> + return -ENOMEM;
> + memcpy(arch->instructions, powerpc__instructions, sizeof(struct
> ins) * arch->nr_instructions);
> + arch->nr_instructions_allocated = arch->nr_instructions;
> arch->initialized = true;
> arch->associate_instruction_ops =
> powerpc__associate_instruction_ops;
> arch->objdump.comment_char = '#';
> diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
> index ac6b8b8da38a..32cf506a9010 100644
> --- a/tools/perf/util/disasm.c
> +++ b/tools/perf/util/disasm.c
> @@ -36,6 +36,7 @@ static struct ins_ops mov_ops;
> static struct ins_ops nop_ops;
> static struct ins_ops lock_ops;
> static struct ins_ops ret_ops;
> +static struct ins_ops load_store_ops;
>
> static int jump__scnprintf(struct ins *ins, char *bf, size_t size,
> struct ins_operands *ops, int max_ins_name);
