Re: [PATCH V3 10/14] tools/perf: Update instruction tracking for powerpc

[Athira Rajeev <atrajeev@linux.vnet.ibm.com>]

> On 6 Jun 2024, at 12:23 PM, Namhyung Kim <namhyung@kernel.org> wrote:
> 
> On Sat, Jun 01, 2024 at 11:39:37AM +0530, Athira Rajeev wrote:
>> Add instruction tracking function "update_insn_state_powerpc" for
>> powerpc. Example sequence in powerpc:
>> 
>> ld      r10,264(r3)
>> mr      r31,r3
>> <<after some sequence>
>> ld      r9,312(r31)
>> 
>> Consider ithe sample is pointing to: "ld r9,312(r31)".
>> Here the memory reference is hit at "312(r31)" where 312 is the offset
>> and r31 is the source register. Previous instruction sequence shows that
>> register state of r3 is moved to r31. So to identify the data type for r31
>> access, the previous instruction ("mr") needs to be tracked and the
>> state type entry has to be updated. Current instruction tracking support
>> in perf tools infrastructure is specific to x86. Patch adds this support
>> for powerpc as well.
>> 
>> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
>> ---
>> .../perf/arch/powerpc/annotate/instructions.c | 65 +++++++++++++++++++
>> tools/perf/util/annotate-data.c               |  9 ++-
>> tools/perf/util/disasm.c                      |  1 +
>> 3 files changed, 74 insertions(+), 1 deletion(-)
>> 
>> diff --git a/tools/perf/arch/powerpc/annotate/instructions.c b/tools/perf/arch/powerpc/annotate/instructions.c
>> index db72148eb857..3ecf5a986037 100644
>> --- a/tools/perf/arch/powerpc/annotate/instructions.c
>> +++ b/tools/perf/arch/powerpc/annotate/instructions.c
>> @@ -231,6 +231,71 @@ static struct ins_ops *check_ppc_insn(int raw_insn)
>> return NULL;
>> }
>> 
>> +/*
>> + * Instruction tracking function to track register state moves.
>> + * Example sequence:
>> + *    ld      r10,264(r3)
>> + *    mr      r31,r3
>> + *    <<after some sequence>
>> + *    ld      r9,312(r31)
>> + *
>> + * Previous instruction sequence shows that register state of r3
>> + * is moved to r31. update_insn_state_powerpc tracks these state
>> + * changes
>> + */
>> +#ifdef HAVE_DWARF_SUPPORT
>> +static void update_insn_state_powerpc(struct type_state *state,
>> + struct data_loc_info *dloc, Dwarf_Die * cu_die __maybe_unused,
>> + struct disasm_line *dl)
>> +{
>> + struct annotated_insn_loc loc;
>> + struct annotated_op_loc *src = &loc.ops[INSN_OP_SOURCE];
>> + struct annotated_op_loc *dst = &loc.ops[INSN_OP_TARGET];
>> + struct type_state_reg *tsr;
>> + u32 insn_offset = dl->al.offset;
>> +
>> + if (annotate_get_insn_location(dloc->arch, dl, &loc) < 0)
>> + return;
>> +
>> + /*
>> +  * Value 444 for bits 21:30 is for "mr"
>> +  * instruction. "mr" is extended OR. So set the
>> +  * source and destination reg correctly
>> +  */
>> + if (PPC_21_30(dl->ops.raw_insn) == 444) {
>> + int src_reg = src->reg1;
>> +
>> + src->reg1 = dst->reg1;
>> + dst->reg1 = src_reg;
>> + }
>> +
>> + if (!has_reg_type(state, dst->reg1))
>> + return;
>> +
>> + tsr = &state->regs[dst->reg1];
>> +
>> + if (!has_reg_type(state, src->reg1) ||
>> + !state->regs[src->reg1].ok) {
>> + tsr->ok = false;
>> + return;
>> + }
>> +
>> + tsr->type = state->regs[src->reg1].type;
>> + tsr->kind = state->regs[src->reg1].kind;
>> + tsr->ok = true;
>> +
>> + pr_debug("mov [%x] reg%d -> reg%d",
> 
> pr_debug_dtp() ?

Sure, will change this in V4

Thanks
Athira
> 
> Thanks,
> Namhyung
> 
> 
>> + insn_offset, src->reg1, dst->reg1);
>> + pr_debug_type_name(&tsr->type, tsr->kind);
>> +}
>> +#else /* HAVE_DWARF_SUPPORT */
>> +static void update_insn_state_powerpc(struct type_state *state __maybe_unused, struct data_loc_info *dloc __maybe_unused,
>> + Dwarf_Die * cu_die __maybe_unused, struct disasm_line *dl __maybe_unused)
>> +{
>> + return;
>> +}
>> +#endif /* HAVE_DWARF_SUPPORT */
>> +
>> static int powerpc__annotate_init(struct arch *arch, char *cpuid __maybe_unused)
>> {
>> if (!arch->initialized) {
>> diff --git a/tools/perf/util/annotate-data.c b/tools/perf/util/annotate-data.c
>> index 7a48c3d72b89..734acdd8c4b7 100644
>> --- a/tools/perf/util/annotate-data.c
>> +++ b/tools/perf/util/annotate-data.c
>> @@ -1080,6 +1080,13 @@ static int find_data_type_insn(struct data_loc_info *dloc,
>> return ret;
>> }
>> 
>> +static int arch_supports_insn_tracking(struct data_loc_info *dloc)
>> +{
>> + if ((arch__is(dloc->arch, "x86")) || (arch__is(dloc->arch, "powerpc")))
>> + return 1;
>> + return 0;
>> +}
>> +
>> /*
>>  * Construct a list of basic blocks for each scope with variables and try to find
>>  * the data type by updating a type state table through instructions.
>> @@ -1094,7 +1101,7 @@ static int find_data_type_block(struct data_loc_info *dloc,
>> int ret = -1;
>> 
>> /* TODO: other architecture support */
>> - if (!arch__is(dloc->arch, "x86"))
>> + if (!arch_supports_insn_tracking(dloc))
>> return -1;
>> 
>> prev_dst_ip = dst_ip = dloc->ip;
>> diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
>> index 57af4dc42a58..d8b357055302 100644
>> --- a/tools/perf/util/disasm.c
>> +++ b/tools/perf/util/disasm.c
>> @@ -155,6 +155,7 @@ static struct arch architectures[] = {
>> {
>> .name = "powerpc",
>> .init = powerpc__annotate_init,
>> + .update_insn_state = update_insn_state_powerpc,
>> },
>> {
>> .name = "riscv64",
>> -- 
>> 2.43.0