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

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

> On 7 May 2024, at 3:18 PM, Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
> 
> 
> 
> 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

Hi Christophe

Sure, I will make these changes

> 
> 
>> + 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.
Ok,, 
> 
> 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

Thanks for pointing this. I checked through others in this list in arch/powerpc/include/asm/ppc-opcode.h 
I will have these filtered in V3

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