[PATCH v2 1/2] perf annotate: Handle x86 instruction suffix generally

[PATCH v2 1/2] perf annotate: Handle x86 instruction suffix generally

[Namhyung Kim]

In AT&T asm syntax, most of x86 instructions can have size suffix like
b, w, l or q.  Instead of adding all these instructions in the table,
we can handle them in a general way.

For example, it can try to find an instruction as is.  If not found,
assuming it has a suffix and it'd try again without the suffix if it's
one of the allowed suffixes.  This way, we can reduce the instruction
table size for duplicated entries of the same instructions with a
different suffix.

If an instruction xyz and others like xyz<suffix> are completely
different ones, then they both need to be listed in the table so that
they can be found before the second attempt (without the suffix).

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/annotate.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index b708bbc49c9e..7f05f2a2aa83 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -70,6 +70,7 @@ struct arch {
 	struct ins_ops  *(*associate_instruction_ops)(struct arch *arch, const char *name);
 	bool		sorted_instructions;
 	bool		initialized;
+	const char	*insn_suffix;
 	void		*priv;
 	unsigned int	model;
 	unsigned int	family;
@@ -179,6 +180,7 @@ static struct arch architectures[] = {
 		.init = x86__annotate_init,
 		.instructions = x86__instructions,
 		.nr_instructions = ARRAY_SIZE(x86__instructions),
+		.insn_suffix = "bwlq",
 		.objdump =  {
 			.comment_char = '#',
 		},
@@ -720,6 +722,26 @@ static struct ins_ops *__ins__find(struct arch *arch, const char *name)
 	}
 
 	ins = bsearch(name, arch->instructions, nmemb, sizeof(struct ins), ins__key_cmp);
+	if (ins)
+		return ins->ops;
+
+	if (arch->insn_suffix) {
+		char tmp[32];
+		char suffix;
+		size_t len = strlen(name);
+
+		if (len == 0 || len >= sizeof(tmp))
+			return NULL;
+
+		suffix = name[len - 1];
+		if (strchr(arch->insn_suffix, suffix) == NULL)
+			return NULL;
+
+		strcpy(tmp, name);
+		tmp[len - 1] = '\0'; /* remove the suffix and check again */
+
+		ins = bsearch(tmp, arch->instructions, nmemb, sizeof(struct ins), ins__key_cmp);
+	}
 	return ins ? ins->ops : NULL;
 }
 
-- 
2.41.0.rc0.172.g3f132b7071-goog

[PATCH v2 2/2] perf annotate: Remove x86 instructions with suffix

[Namhyung Kim]

Now the suffix is handled in the general code.  Let's get rid of them.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/arch/x86/annotate/instructions.c | 52 ++++-----------------
 1 file changed, 10 insertions(+), 42 deletions(-)

diff --git a/tools/perf/arch/x86/annotate/instructions.c b/tools/perf/arch/x86/annotate/instructions.c
index 5c7bec25fee4..5f4ac4fc7fcf 100644
--- a/tools/perf/arch/x86/annotate/instructions.c
+++ b/tools/perf/arch/x86/annotate/instructions.c
@@ -1,48 +1,37 @@
 // SPDX-License-Identifier: GPL-2.0
+/*
+ * x86 instruction nmemonic table to parse disasm lines for annotate.
+ * This table is searched twice - one for exact match and another for
+ * match without a size suffix (b, w, l, q) in case of AT&T syntax.
+ *
+ * So this table should not have entries with the suffix unless it's
+ * a complete different instruction than ones without the suffix.
+ */
 static struct ins x86__instructions[] = {
 	{ .name = "adc",	.ops = &mov_ops,  },
-	{ .name = "adcb",	.ops = &mov_ops,  },
-	{ .name = "adcl",	.ops = &mov_ops,  },
 	{ .name = "add",	.ops = &mov_ops,  },
-	{ .name = "addl",	.ops = &mov_ops,  },
-	{ .name = "addq",	.ops = &mov_ops,  },
 	{ .name = "addsd",	.ops = &mov_ops,  },
-	{ .name = "addw",	.ops = &mov_ops,  },
 	{ .name = "and",	.ops = &mov_ops,  },
-	{ .name = "andb",	.ops = &mov_ops,  },
-	{ .name = "andl",	.ops = &mov_ops,  },
 	{ .name = "andpd",	.ops = &mov_ops,  },
 	{ .name = "andps",	.ops = &mov_ops,  },
-	{ .name = "andq",	.ops = &mov_ops,  },
-	{ .name = "andw",	.ops = &mov_ops,  },
 	{ .name = "bsr",	.ops = &mov_ops,  },
 	{ .name = "bt",		.ops = &mov_ops,  },
 	{ .name = "btr",	.ops = &mov_ops,  },
 	{ .name = "bts",	.ops = &mov_ops,  },
-	{ .name = "btsq",	.ops = &mov_ops,  },
 	{ .name = "call",	.ops = &call_ops, },
-	{ .name = "callq",	.ops = &call_ops, },
 	{ .name = "cmovbe",	.ops = &mov_ops,  },
 	{ .name = "cmove",	.ops = &mov_ops,  },
 	{ .name = "cmovae",	.ops = &mov_ops,  },
 	{ .name = "cmp",	.ops = &mov_ops,  },
-	{ .name = "cmpb",	.ops = &mov_ops,  },
-	{ .name = "cmpl",	.ops = &mov_ops,  },
-	{ .name = "cmpq",	.ops = &mov_ops,  },
-	{ .name = "cmpw",	.ops = &mov_ops,  },
 	{ .name = "cmpxch",	.ops = &mov_ops,  },
 	{ .name = "cmpxchg",	.ops = &mov_ops,  },
 	{ .name = "cs",		.ops = &mov_ops,  },
 	{ .name = "dec",	.ops = &dec_ops,  },
-	{ .name = "decl",	.ops = &dec_ops,  },
-	{ .name = "decq",	.ops = &dec_ops,  },
 	{ .name = "divsd",	.ops = &mov_ops,  },
 	{ .name = "divss",	.ops = &mov_ops,  },
 	{ .name = "gs",		.ops = &mov_ops,  },
 	{ .name = "imul",	.ops = &mov_ops,  },
 	{ .name = "inc",	.ops = &dec_ops,  },
-	{ .name = "incl",	.ops = &dec_ops,  },
-	{ .name = "incq",	.ops = &dec_ops,  },
 	{ .name = "ja",		.ops = &jump_ops, },
 	{ .name = "jae",	.ops = &jump_ops, },
 	{ .name = "jb",		.ops = &jump_ops, },
@@ -56,7 +45,6 @@ static struct ins x86__instructions[] = {
 	{ .name = "jl",		.ops = &jump_ops, },
 	{ .name = "jle",	.ops = &jump_ops, },
 	{ .name = "jmp",	.ops = &jump_ops, },
-	{ .name = "jmpq",	.ops = &jump_ops, },
 	{ .name = "jna",	.ops = &jump_ops, },
 	{ .name = "jnae",	.ops = &jump_ops, },
 	{ .name = "jnb",	.ops = &jump_ops, },
@@ -83,49 +71,31 @@ static struct ins x86__instructions[] = {
 	{ .name = "mov",	.ops = &mov_ops,  },
 	{ .name = "movapd",	.ops = &mov_ops,  },
 	{ .name = "movaps",	.ops = &mov_ops,  },
-	{ .name = "movb",	.ops = &mov_ops,  },
 	{ .name = "movdqa",	.ops = &mov_ops,  },
 	{ .name = "movdqu",	.ops = &mov_ops,  },
-	{ .name = "movl",	.ops = &mov_ops,  },
-	{ .name = "movq",	.ops = &mov_ops,  },
 	{ .name = "movsd",	.ops = &mov_ops,  },
 	{ .name = "movslq",	.ops = &mov_ops,  },
 	{ .name = "movss",	.ops = &mov_ops,  },
 	{ .name = "movupd",	.ops = &mov_ops,  },
 	{ .name = "movups",	.ops = &mov_ops,  },
-	{ .name = "movw",	.ops = &mov_ops,  },
 	{ .name = "movzbl",	.ops = &mov_ops,  },
 	{ .name = "movzwl",	.ops = &mov_ops,  },
 	{ .name = "mulsd",	.ops = &mov_ops,  },
 	{ .name = "mulss",	.ops = &mov_ops,  },
 	{ .name = "nop",	.ops = &nop_ops,  },
-	{ .name = "nopl",	.ops = &nop_ops,  },
-	{ .name = "nopw",	.ops = &nop_ops,  },
 	{ .name = "or",		.ops = &mov_ops,  },
-	{ .name = "orb",	.ops = &mov_ops,  },
-	{ .name = "orl",	.ops = &mov_ops,  },
 	{ .name = "orps",	.ops = &mov_ops,  },
-	{ .name = "orq",	.ops = &mov_ops,  },
 	{ .name = "pand",	.ops = &mov_ops,  },
 	{ .name = "paddq",	.ops = &mov_ops,  },
 	{ .name = "pcmpeqb",	.ops = &mov_ops,  },
 	{ .name = "por",	.ops = &mov_ops,  },
-	{ .name = "rclb",	.ops = &mov_ops,  },
-	{ .name = "rcll",	.ops = &mov_ops,  },
+	{ .name = "rcl",	.ops = &mov_ops,  },
 	{ .name = "ret",	.ops = &ret_ops,  },
-	{ .name = "retq",	.ops = &ret_ops,  },
 	{ .name = "sbb",	.ops = &mov_ops,  },
-	{ .name = "sbbl",	.ops = &mov_ops,  },
 	{ .name = "sete",	.ops = &mov_ops,  },
 	{ .name = "sub",	.ops = &mov_ops,  },
-	{ .name = "subl",	.ops = &mov_ops,  },
-	{ .name = "subq",	.ops = &mov_ops,  },
 	{ .name = "subsd",	.ops = &mov_ops,  },
-	{ .name = "subw",	.ops = &mov_ops,  },
 	{ .name = "test",	.ops = &mov_ops,  },
-	{ .name = "testb",	.ops = &mov_ops,  },
-	{ .name = "testl",	.ops = &mov_ops,  },
-	{ .name = "testq",	.ops = &mov_ops,  },
 	{ .name = "tzcnt",	.ops = &mov_ops,  },
 	{ .name = "ucomisd",	.ops = &mov_ops,  },
 	{ .name = "ucomiss",	.ops = &mov_ops,  },
@@ -139,11 +109,9 @@ static struct ins x86__instructions[] = {
 	{ .name = "vsubsd",	.ops = &mov_ops,  },
 	{ .name = "vucomisd",	.ops = &mov_ops,  },
 	{ .name = "xadd",	.ops = &mov_ops,  },
-	{ .name = "xbeginl",	.ops = &jump_ops, },
-	{ .name = "xbeginq",	.ops = &jump_ops, },
+	{ .name = "xbegin",	.ops = &jump_ops, },
 	{ .name = "xchg",	.ops = &mov_ops,  },
 	{ .name = "xor",	.ops = &mov_ops, },
-	{ .name = "xorb",	.ops = &mov_ops, },
 	{ .name = "xorpd",	.ops = &mov_ops, },
 	{ .name = "xorps",	.ops = &mov_ops, },
 };
-- 
2.41.0.rc0.172.g3f132b7071-goog

Re: [PATCH v2 2/2] perf annotate: Remove x86 instructions with suffix

[Adrian Hunter]

On 24/05/23 23:50, Namhyung Kim wrote:
> Now the suffix is handled in the general code.  Let's get rid of them.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  tools/perf/arch/x86/annotate/instructions.c | 52 ++++-----------------
>  1 file changed, 10 insertions(+), 42 deletions(-)
> 
> diff --git a/tools/perf/arch/x86/annotate/instructions.c b/tools/perf/arch/x86/annotate/instructions.c
> index 5c7bec25fee4..5f4ac4fc7fcf 100644
> --- a/tools/perf/arch/x86/annotate/instructions.c
> +++ b/tools/perf/arch/x86/annotate/instructions.c
> @@ -1,48 +1,37 @@
>  // SPDX-License-Identifier: GPL-2.0
> +/*
> + * x86 instruction nmemonic table to parse disasm lines for annotate.
> + * This table is searched twice - one for exact match and another for
> + * match without a size suffix (b, w, l, q) in case of AT&T syntax.
> + *
> + * So this table should not have entries with the suffix unless it's
> + * a complete different instruction than ones without the suffix.
> + */
>  static struct ins x86__instructions[] = {
>  	{ .name = "adc",	.ops = &mov_ops,  },
> -	{ .name = "adcb",	.ops = &mov_ops,  },
> -	{ .name = "adcl",	.ops = &mov_ops,  },
>  	{ .name = "add",	.ops = &mov_ops,  },
> -	{ .name = "addl",	.ops = &mov_ops,  },
> -	{ .name = "addq",	.ops = &mov_ops,  },
>  	{ .name = "addsd",	.ops = &mov_ops,  },
> -	{ .name = "addw",	.ops = &mov_ops,  },
>  	{ .name = "and",	.ops = &mov_ops,  },
> -	{ .name = "andb",	.ops = &mov_ops,  },
> -	{ .name = "andl",	.ops = &mov_ops,  },
>  	{ .name = "andpd",	.ops = &mov_ops,  },
>  	{ .name = "andps",	.ops = &mov_ops,  },
> -	{ .name = "andq",	.ops = &mov_ops,  },
> -	{ .name = "andw",	.ops = &mov_ops,  },
>  	{ .name = "bsr",	.ops = &mov_ops,  },
>  	{ .name = "bt",		.ops = &mov_ops,  },
>  	{ .name = "btr",	.ops = &mov_ops,  },
>  	{ .name = "bts",	.ops = &mov_ops,  },
> -	{ .name = "btsq",	.ops = &mov_ops,  },
>  	{ .name = "call",	.ops = &call_ops, },
> -	{ .name = "callq",	.ops = &call_ops, },
>  	{ .name = "cmovbe",	.ops = &mov_ops,  },
>  	{ .name = "cmove",	.ops = &mov_ops,  },
>  	{ .name = "cmovae",	.ops = &mov_ops,  },
>  	{ .name = "cmp",	.ops = &mov_ops,  },
> -	{ .name = "cmpb",	.ops = &mov_ops,  },
> -	{ .name = "cmpl",	.ops = &mov_ops,  },
> -	{ .name = "cmpq",	.ops = &mov_ops,  },
> -	{ .name = "cmpw",	.ops = &mov_ops,  },
>  	{ .name = "cmpxch",	.ops = &mov_ops,  },
>  	{ .name = "cmpxchg",	.ops = &mov_ops,  },
>  	{ .name = "cs",		.ops = &mov_ops,  },
>  	{ .name = "dec",	.ops = &dec_ops,  },
> -	{ .name = "decl",	.ops = &dec_ops,  },
> -	{ .name = "decq",	.ops = &dec_ops,  },
>  	{ .name = "divsd",	.ops = &mov_ops,  },
>  	{ .name = "divss",	.ops = &mov_ops,  },
>  	{ .name = "gs",		.ops = &mov_ops,  },
>  	{ .name = "imul",	.ops = &mov_ops,  },
>  	{ .name = "inc",	.ops = &dec_ops,  },
> -	{ .name = "incl",	.ops = &dec_ops,  },
> -	{ .name = "incq",	.ops = &dec_ops,  },
>  	{ .name = "ja",		.ops = &jump_ops, },
>  	{ .name = "jae",	.ops = &jump_ops, },
>  	{ .name = "jb",		.ops = &jump_ops, },
> @@ -56,7 +45,6 @@ static struct ins x86__instructions[] = {
>  	{ .name = "jl",		.ops = &jump_ops, },
>  	{ .name = "jle",	.ops = &jump_ops, },
>  	{ .name = "jmp",	.ops = &jump_ops, },
> -	{ .name = "jmpq",	.ops = &jump_ops, },
>  	{ .name = "jna",	.ops = &jump_ops, },
>  	{ .name = "jnae",	.ops = &jump_ops, },
>  	{ .name = "jnb",	.ops = &jump_ops, },
> @@ -83,49 +71,31 @@ static struct ins x86__instructions[] = {
>  	{ .name = "mov",	.ops = &mov_ops,  },
>  	{ .name = "movapd",	.ops = &mov_ops,  },
>  	{ .name = "movaps",	.ops = &mov_ops,  },
> -	{ .name = "movb",	.ops = &mov_ops,  },
>  	{ .name = "movdqa",	.ops = &mov_ops,  },
>  	{ .name = "movdqu",	.ops = &mov_ops,  },
> -	{ .name = "movl",	.ops = &mov_ops,  },
> -	{ .name = "movq",	.ops = &mov_ops,  },
>  	{ .name = "movsd",	.ops = &mov_ops,  },
>  	{ .name = "movslq",	.ops = &mov_ops,  },
>  	{ .name = "movss",	.ops = &mov_ops,  },
>  	{ .name = "movupd",	.ops = &mov_ops,  },
>  	{ .name = "movups",	.ops = &mov_ops,  },
> -	{ .name = "movw",	.ops = &mov_ops,  },
>  	{ .name = "movzbl",	.ops = &mov_ops,  },
>  	{ .name = "movzwl",	.ops = &mov_ops,  },
>  	{ .name = "mulsd",	.ops = &mov_ops,  },
>  	{ .name = "mulss",	.ops = &mov_ops,  },
>  	{ .name = "nop",	.ops = &nop_ops,  },
> -	{ .name = "nopl",	.ops = &nop_ops,  },
> -	{ .name = "nopw",	.ops = &nop_ops,  },
>  	{ .name = "or",		.ops = &mov_ops,  },
> -	{ .name = "orb",	.ops = &mov_ops,  },
> -	{ .name = "orl",	.ops = &mov_ops,  },
>  	{ .name = "orps",	.ops = &mov_ops,  },
> -	{ .name = "orq",	.ops = &mov_ops,  },
>  	{ .name = "pand",	.ops = &mov_ops,  },
>  	{ .name = "paddq",	.ops = &mov_ops,  },
>  	{ .name = "pcmpeqb",	.ops = &mov_ops,  },
>  	{ .name = "por",	.ops = &mov_ops,  },
> -	{ .name = "rclb",	.ops = &mov_ops,  },
> -	{ .name = "rcll",	.ops = &mov_ops,  },
> +	{ .name = "rcl",	.ops = &mov_ops,  },
>  	{ .name = "ret",	.ops = &ret_ops,  },
> -	{ .name = "retq",	.ops = &ret_ops,  },
>  	{ .name = "sbb",	.ops = &mov_ops,  },
> -	{ .name = "sbbl",	.ops = &mov_ops,  },
>  	{ .name = "sete",	.ops = &mov_ops,  },
>  	{ .name = "sub",	.ops = &mov_ops,  },
> -	{ .name = "subl",	.ops = &mov_ops,  },
> -	{ .name = "subq",	.ops = &mov_ops,  },
>  	{ .name = "subsd",	.ops = &mov_ops,  },
> -	{ .name = "subw",	.ops = &mov_ops,  },
>  	{ .name = "test",	.ops = &mov_ops,  },
> -	{ .name = "testb",	.ops = &mov_ops,  },
> -	{ .name = "testl",	.ops = &mov_ops,  },
> -	{ .name = "testq",	.ops = &mov_ops,  },
>  	{ .name = "tzcnt",	.ops = &mov_ops,  },
>  	{ .name = "ucomisd",	.ops = &mov_ops,  },
>  	{ .name = "ucomiss",	.ops = &mov_ops,  },
> @@ -139,11 +109,9 @@ static struct ins x86__instructions[] = {
>  	{ .name = "vsubsd",	.ops = &mov_ops,  },
>  	{ .name = "vucomisd",	.ops = &mov_ops,  },
>  	{ .name = "xadd",	.ops = &mov_ops,  },
> -	{ .name = "xbeginl",	.ops = &jump_ops, },
> -	{ .name = "xbeginq",	.ops = &jump_ops, },
> +	{ .name = "xbegin",	.ops = &jump_ops, },
>  	{ .name = "xchg",	.ops = &mov_ops,  },
>  	{ .name = "xor",	.ops = &mov_ops, },
> -	{ .name = "xorb",	.ops = &mov_ops, },
>  	{ .name = "xorpd",	.ops = &mov_ops, },
>  	{ .name = "xorps",	.ops = &mov_ops, },
>  };

Re: [PATCH v2 2/2] perf annotate: Remove x86 instructions with suffix

[Masami Hiramatsu]

On Wed, 24 May 2023 13:50:54 -0700
Namhyung Kim <namhyung@kernel.org> wrote:

> Now the suffix is handled in the general code.  Let's get rid of them.
> 

Looks good to me.

Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>


> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/arch/x86/annotate/instructions.c | 52 ++++-----------------
>  1 file changed, 10 insertions(+), 42 deletions(-)
> 
> diff --git a/tools/perf/arch/x86/annotate/instructions.c b/tools/perf/arch/x86/annotate/instructions.c
> index 5c7bec25fee4..5f4ac4fc7fcf 100644
> --- a/tools/perf/arch/x86/annotate/instructions.c
> +++ b/tools/perf/arch/x86/annotate/instructions.c
> @@ -1,48 +1,37 @@
>  // SPDX-License-Identifier: GPL-2.0
> +/*
> + * x86 instruction nmemonic table to parse disasm lines for annotate.
> + * This table is searched twice - one for exact match and another for
> + * match without a size suffix (b, w, l, q) in case of AT&T syntax.
> + *
> + * So this table should not have entries with the suffix unless it's
> + * a complete different instruction than ones without the suffix.
> + */
>  static struct ins x86__instructions[] = {
>  	{ .name = "adc",	.ops = &mov_ops,  },
> -	{ .name = "adcb",	.ops = &mov_ops,  },
> -	{ .name = "adcl",	.ops = &mov_ops,  },
>  	{ .name = "add",	.ops = &mov_ops,  },
> -	{ .name = "addl",	.ops = &mov_ops,  },
> -	{ .name = "addq",	.ops = &mov_ops,  },
>  	{ .name = "addsd",	.ops = &mov_ops,  },
> -	{ .name = "addw",	.ops = &mov_ops,  },
>  	{ .name = "and",	.ops = &mov_ops,  },
> -	{ .name = "andb",	.ops = &mov_ops,  },
> -	{ .name = "andl",	.ops = &mov_ops,  },
>  	{ .name = "andpd",	.ops = &mov_ops,  },
>  	{ .name = "andps",	.ops = &mov_ops,  },
> -	{ .name = "andq",	.ops = &mov_ops,  },
> -	{ .name = "andw",	.ops = &mov_ops,  },
>  	{ .name = "bsr",	.ops = &mov_ops,  },
>  	{ .name = "bt",		.ops = &mov_ops,  },
>  	{ .name = "btr",	.ops = &mov_ops,  },
>  	{ .name = "bts",	.ops = &mov_ops,  },
> -	{ .name = "btsq",	.ops = &mov_ops,  },
>  	{ .name = "call",	.ops = &call_ops, },
> -	{ .name = "callq",	.ops = &call_ops, },
>  	{ .name = "cmovbe",	.ops = &mov_ops,  },
>  	{ .name = "cmove",	.ops = &mov_ops,  },
>  	{ .name = "cmovae",	.ops = &mov_ops,  },
>  	{ .name = "cmp",	.ops = &mov_ops,  },
> -	{ .name = "cmpb",	.ops = &mov_ops,  },
> -	{ .name = "cmpl",	.ops = &mov_ops,  },
> -	{ .name = "cmpq",	.ops = &mov_ops,  },
> -	{ .name = "cmpw",	.ops = &mov_ops,  },
>  	{ .name = "cmpxch",	.ops = &mov_ops,  },
>  	{ .name = "cmpxchg",	.ops = &mov_ops,  },
>  	{ .name = "cs",		.ops = &mov_ops,  },
>  	{ .name = "dec",	.ops = &dec_ops,  },
> -	{ .name = "decl",	.ops = &dec_ops,  },
> -	{ .name = "decq",	.ops = &dec_ops,  },
>  	{ .name = "divsd",	.ops = &mov_ops,  },
>  	{ .name = "divss",	.ops = &mov_ops,  },
>  	{ .name = "gs",		.ops = &mov_ops,  },
>  	{ .name = "imul",	.ops = &mov_ops,  },
>  	{ .name = "inc",	.ops = &dec_ops,  },
> -	{ .name = "incl",	.ops = &dec_ops,  },
> -	{ .name = "incq",	.ops = &dec_ops,  },
>  	{ .name = "ja",		.ops = &jump_ops, },
>  	{ .name = "jae",	.ops = &jump_ops, },
>  	{ .name = "jb",		.ops = &jump_ops, },
> @@ -56,7 +45,6 @@ static struct ins x86__instructions[] = {
>  	{ .name = "jl",		.ops = &jump_ops, },
>  	{ .name = "jle",	.ops = &jump_ops, },
>  	{ .name = "jmp",	.ops = &jump_ops, },
> -	{ .name = "jmpq",	.ops = &jump_ops, },
>  	{ .name = "jna",	.ops = &jump_ops, },
>  	{ .name = "jnae",	.ops = &jump_ops, },
>  	{ .name = "jnb",	.ops = &jump_ops, },
> @@ -83,49 +71,31 @@ static struct ins x86__instructions[] = {
>  	{ .name = "mov",	.ops = &mov_ops,  },
>  	{ .name = "movapd",	.ops = &mov_ops,  },
>  	{ .name = "movaps",	.ops = &mov_ops,  },
> -	{ .name = "movb",	.ops = &mov_ops,  },
>  	{ .name = "movdqa",	.ops = &mov_ops,  },
>  	{ .name = "movdqu",	.ops = &mov_ops,  },
> -	{ .name = "movl",	.ops = &mov_ops,  },
> -	{ .name = "movq",	.ops = &mov_ops,  },
>  	{ .name = "movsd",	.ops = &mov_ops,  },
>  	{ .name = "movslq",	.ops = &mov_ops,  },
>  	{ .name = "movss",	.ops = &mov_ops,  },
>  	{ .name = "movupd",	.ops = &mov_ops,  },
>  	{ .name = "movups",	.ops = &mov_ops,  },
> -	{ .name = "movw",	.ops = &mov_ops,  },
>  	{ .name = "movzbl",	.ops = &mov_ops,  },
>  	{ .name = "movzwl",	.ops = &mov_ops,  },
>  	{ .name = "mulsd",	.ops = &mov_ops,  },
>  	{ .name = "mulss",	.ops = &mov_ops,  },
>  	{ .name = "nop",	.ops = &nop_ops,  },
> -	{ .name = "nopl",	.ops = &nop_ops,  },
> -	{ .name = "nopw",	.ops = &nop_ops,  },
>  	{ .name = "or",		.ops = &mov_ops,  },
> -	{ .name = "orb",	.ops = &mov_ops,  },
> -	{ .name = "orl",	.ops = &mov_ops,  },
>  	{ .name = "orps",	.ops = &mov_ops,  },
> -	{ .name = "orq",	.ops = &mov_ops,  },
>  	{ .name = "pand",	.ops = &mov_ops,  },
>  	{ .name = "paddq",	.ops = &mov_ops,  },
>  	{ .name = "pcmpeqb",	.ops = &mov_ops,  },
>  	{ .name = "por",	.ops = &mov_ops,  },
> -	{ .name = "rclb",	.ops = &mov_ops,  },
> -	{ .name = "rcll",	.ops = &mov_ops,  },
> +	{ .name = "rcl",	.ops = &mov_ops,  },
>  	{ .name = "ret",	.ops = &ret_ops,  },
> -	{ .name = "retq",	.ops = &ret_ops,  },
>  	{ .name = "sbb",	.ops = &mov_ops,  },
> -	{ .name = "sbbl",	.ops = &mov_ops,  },
>  	{ .name = "sete",	.ops = &mov_ops,  },
>  	{ .name = "sub",	.ops = &mov_ops,  },
> -	{ .name = "subl",	.ops = &mov_ops,  },
> -	{ .name = "subq",	.ops = &mov_ops,  },
>  	{ .name = "subsd",	.ops = &mov_ops,  },
> -	{ .name = "subw",	.ops = &mov_ops,  },
>  	{ .name = "test",	.ops = &mov_ops,  },
> -	{ .name = "testb",	.ops = &mov_ops,  },
> -	{ .name = "testl",	.ops = &mov_ops,  },
> -	{ .name = "testq",	.ops = &mov_ops,  },
>  	{ .name = "tzcnt",	.ops = &mov_ops,  },
>  	{ .name = "ucomisd",	.ops = &mov_ops,  },
>  	{ .name = "ucomiss",	.ops = &mov_ops,  },
> @@ -139,11 +109,9 @@ static struct ins x86__instructions[] = {
>  	{ .name = "vsubsd",	.ops = &mov_ops,  },
>  	{ .name = "vucomisd",	.ops = &mov_ops,  },
>  	{ .name = "xadd",	.ops = &mov_ops,  },
> -	{ .name = "xbeginl",	.ops = &jump_ops, },
> -	{ .name = "xbeginq",	.ops = &jump_ops, },
> +	{ .name = "xbegin",	.ops = &jump_ops, },
>  	{ .name = "xchg",	.ops = &mov_ops,  },
>  	{ .name = "xor",	.ops = &mov_ops, },
> -	{ .name = "xorb",	.ops = &mov_ops, },
>  	{ .name = "xorpd",	.ops = &mov_ops, },
>  	{ .name = "xorps",	.ops = &mov_ops, },
>  };
> -- 
> 2.41.0.rc0.172.g3f132b7071-goog
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v2 1/2] perf annotate: Handle x86 instruction suffix generally

[Adrian Hunter]

On 24/05/23 23:50, Namhyung Kim wrote:
> In AT&T asm syntax, most of x86 instructions can have size suffix like
> b, w, l or q.  Instead of adding all these instructions in the table,
> we can handle them in a general way.
> 
> For example, it can try to find an instruction as is.  If not found,
> assuming it has a suffix and it'd try again without the suffix if it's
> one of the allowed suffixes.  This way, we can reduce the instruction
> table size for duplicated entries of the same instructions with a
> different suffix.
> 
> If an instruction xyz and others like xyz<suffix> are completely
> different ones, then they both need to be listed in the table so that
> they can be found before the second attempt (without the suffix).
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  tools/perf/util/annotate.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index b708bbc49c9e..7f05f2a2aa83 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -70,6 +70,7 @@ struct arch {
>  	struct ins_ops  *(*associate_instruction_ops)(struct arch *arch, const char *name);
>  	bool		sorted_instructions;
>  	bool		initialized;
> +	const char	*insn_suffix;
>  	void		*priv;
>  	unsigned int	model;
>  	unsigned int	family;
> @@ -179,6 +180,7 @@ static struct arch architectures[] = {
>  		.init = x86__annotate_init,
>  		.instructions = x86__instructions,
>  		.nr_instructions = ARRAY_SIZE(x86__instructions),
> +		.insn_suffix = "bwlq",
>  		.objdump =  {
>  			.comment_char = '#',
>  		},
> @@ -720,6 +722,26 @@ static struct ins_ops *__ins__find(struct arch *arch, const char *name)
>  	}
>  
>  	ins = bsearch(name, arch->instructions, nmemb, sizeof(struct ins), ins__key_cmp);
> +	if (ins)
> +		return ins->ops;
> +
> +	if (arch->insn_suffix) {
> +		char tmp[32];
> +		char suffix;
> +		size_t len = strlen(name);
> +
> +		if (len == 0 || len >= sizeof(tmp))
> +			return NULL;
> +
> +		suffix = name[len - 1];
> +		if (strchr(arch->insn_suffix, suffix) == NULL)
> +			return NULL;
> +
> +		strcpy(tmp, name);
> +		tmp[len - 1] = '\0'; /* remove the suffix and check again */
> +
> +		ins = bsearch(tmp, arch->instructions, nmemb, sizeof(struct ins), ins__key_cmp);
> +	}
>  	return ins ? ins->ops : NULL;
>  }
>  

Re: [PATCH v2 1/2] perf annotate: Handle x86 instruction suffix generally

[Namhyung Kim]

Hi Arnaldo,

On Wed, May 24, 2023 at 10:21 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 24/05/23 23:50, Namhyung Kim wrote:
> > In AT&T asm syntax, most of x86 instructions can have size suffix like
> > b, w, l or q.  Instead of adding all these instructions in the table,
> > we can handle them in a general way.
> >
> > For example, it can try to find an instruction as is.  If not found,
> > assuming it has a suffix and it'd try again without the suffix if it's
> > one of the allowed suffixes.  This way, we can reduce the instruction
> > table size for duplicated entries of the same instructions with a
> > different suffix.
> >
> > If an instruction xyz and others like xyz<suffix> are completely
> > different ones, then they both need to be listed in the table so that
> > they can be found before the second attempt (without the suffix).
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>
> Reviewed-by: Adrian Hunter <adrian.hunter@intel.com>

Can you please pick this up?

Thanks,
Namhyung

>
> > ---
> >  tools/perf/util/annotate.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> >
> > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> > index b708bbc49c9e..7f05f2a2aa83 100644
> > --- a/tools/perf/util/annotate.c
> > +++ b/tools/perf/util/annotate.c
> > @@ -70,6 +70,7 @@ struct arch {
> >       struct ins_ops  *(*associate_instruction_ops)(struct arch *arch, const char *name);
> >       bool            sorted_instructions;
> >       bool            initialized;
> > +     const char      *insn_suffix;
> >       void            *priv;
> >       unsigned int    model;
> >       unsigned int    family;
> > @@ -179,6 +180,7 @@ static struct arch architectures[] = {
> >               .init = x86__annotate_init,
> >               .instructions = x86__instructions,
> >               .nr_instructions = ARRAY_SIZE(x86__instructions),
> > +             .insn_suffix = "bwlq",
> >               .objdump =  {
> >                       .comment_char = '#',
> >               },
> > @@ -720,6 +722,26 @@ static struct ins_ops *__ins__find(struct arch *arch, const char *name)
> >       }
> >
> >       ins = bsearch(name, arch->instructions, nmemb, sizeof(struct ins), ins__key_cmp);
> > +     if (ins)
> > +             return ins->ops;
> > +
> > +     if (arch->insn_suffix) {
> > +             char tmp[32];
> > +             char suffix;
> > +             size_t len = strlen(name);
> > +
> > +             if (len == 0 || len >= sizeof(tmp))
> > +                     return NULL;
> > +
> > +             suffix = name[len - 1];
> > +             if (strchr(arch->insn_suffix, suffix) == NULL)
> > +                     return NULL;
> > +
> > +             strcpy(tmp, name);
> > +             tmp[len - 1] = '\0'; /* remove the suffix and check again */
> > +
> > +             ins = bsearch(tmp, arch->instructions, nmemb, sizeof(struct ins), ins__key_cmp);
> > +     }
> >       return ins ? ins->ops : NULL;
> >  }
> >
>

Re: [PATCH v2 1/2] perf annotate: Handle x86 instruction suffix generally

[Masami Hiramatsu]

On Wed, 24 May 2023 13:50:53 -0700
Namhyung Kim <namhyung@kernel.org> wrote:

> In AT&T asm syntax, most of x86 instructions can have size suffix like
> b, w, l or q.  Instead of adding all these instructions in the table,
> we can handle them in a general way.
> 
> For example, it can try to find an instruction as is.  If not found,
> assuming it has a suffix and it'd try again without the suffix if it's
> one of the allowed suffixes.  This way, we can reduce the instruction
> table size for duplicated entries of the same instructions with a
> different suffix.
> 
> If an instruction xyz and others like xyz<suffix> are completely
> different ones, then they both need to be listed in the table so that
> they can be found before the second attempt (without the suffix).

Looks good to me.

Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/annotate.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index b708bbc49c9e..7f05f2a2aa83 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -70,6 +70,7 @@ struct arch {
>  	struct ins_ops  *(*associate_instruction_ops)(struct arch *arch, const char *name);
>  	bool		sorted_instructions;
>  	bool		initialized;
> +	const char	*insn_suffix;
>  	void		*priv;
>  	unsigned int	model;
>  	unsigned int	family;
> @@ -179,6 +180,7 @@ static struct arch architectures[] = {
>  		.init = x86__annotate_init,
>  		.instructions = x86__instructions,
>  		.nr_instructions = ARRAY_SIZE(x86__instructions),
> +		.insn_suffix = "bwlq",
>  		.objdump =  {
>  			.comment_char = '#',
>  		},
> @@ -720,6 +722,26 @@ static struct ins_ops *__ins__find(struct arch *arch, const char *name)
>  	}
>  
>  	ins = bsearch(name, arch->instructions, nmemb, sizeof(struct ins), ins__key_cmp);
> +	if (ins)
> +		return ins->ops;
> +
> +	if (arch->insn_suffix) {
> +		char tmp[32];
> +		char suffix;
> +		size_t len = strlen(name);
> +
> +		if (len == 0 || len >= sizeof(tmp))
> +			return NULL;
> +
> +		suffix = name[len - 1];
> +		if (strchr(arch->insn_suffix, suffix) == NULL)
> +			return NULL;
> +
> +		strcpy(tmp, name);
> +		tmp[len - 1] = '\0'; /* remove the suffix and check again */
> +
> +		ins = bsearch(tmp, arch->instructions, nmemb, sizeof(struct ins), ins__key_cmp);
> +	}
>  	return ins ? ins->ops : NULL;
>  }
>  
> -- 
> 2.41.0.rc0.172.g3f132b7071-goog
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v2 1/2] perf annotate: Handle x86 instruction suffix generally

[Arnaldo Carvalho de Melo]

Em Tue, Jun 06, 2023 at 11:06:58PM +0900, Masami Hiramatsu escreveu:
> On Wed, 24 May 2023 13:50:53 -0700
> Namhyung Kim <namhyung@kernel.org> wrote:
> 
> > In AT&T asm syntax, most of x86 instructions can have size suffix like
> > b, w, l or q.  Instead of adding all these instructions in the table,
> > we can handle them in a general way.
> > 
> > For example, it can try to find an instruction as is.  If not found,
> > assuming it has a suffix and it'd try again without the suffix if it's
> > one of the allowed suffixes.  This way, we can reduce the instruction
> > table size for duplicated entries of the same instructions with a
> > different suffix.
> > 
> > If an instruction xyz and others like xyz<suffix> are completely
> > different ones, then they both need to be listed in the table so that
> > they can be found before the second attempt (without the suffix).
> 
> Looks good to me.
> 
> Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Thanks, applied both patches.

- Arnaldo

 
> > 
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/util/annotate.c | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> > 
> > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> > index b708bbc49c9e..7f05f2a2aa83 100644
> > --- a/tools/perf/util/annotate.c
> > +++ b/tools/perf/util/annotate.c
> > @@ -70,6 +70,7 @@ struct arch {
> >  	struct ins_ops  *(*associate_instruction_ops)(struct arch *arch, const char *name);
> >  	bool		sorted_instructions;
> >  	bool		initialized;
> > +	const char	*insn_suffix;
> >  	void		*priv;
> >  	unsigned int	model;
> >  	unsigned int	family;
> > @@ -179,6 +180,7 @@ static struct arch architectures[] = {
> >  		.init = x86__annotate_init,
> >  		.instructions = x86__instructions,
> >  		.nr_instructions = ARRAY_SIZE(x86__instructions),
> > +		.insn_suffix = "bwlq",
> >  		.objdump =  {
> >  			.comment_char = '#',
> >  		},
> > @@ -720,6 +722,26 @@ static struct ins_ops *__ins__find(struct arch *arch, const char *name)
> >  	}
> >  
> >  	ins = bsearch(name, arch->instructions, nmemb, sizeof(struct ins), ins__key_cmp);
> > +	if (ins)
> > +		return ins->ops;
> > +
> > +	if (arch->insn_suffix) {
> > +		char tmp[32];
> > +		char suffix;
> > +		size_t len = strlen(name);
> > +
> > +		if (len == 0 || len >= sizeof(tmp))
> > +			return NULL;
> > +
> > +		suffix = name[len - 1];
> > +		if (strchr(arch->insn_suffix, suffix) == NULL)
> > +			return NULL;
> > +
> > +		strcpy(tmp, name);
> > +		tmp[len - 1] = '\0'; /* remove the suffix and check again */
> > +
> > +		ins = bsearch(tmp, arch->instructions, nmemb, sizeof(struct ins), ins__key_cmp);
> > +	}
> >  	return ins ? ins->ops : NULL;
> >  }
> >  
> > -- 
> > 2.41.0.rc0.172.g3f132b7071-goog
> > 
> 
> 
> -- 
> Masami Hiramatsu (Google) <mhiramat@kernel.org>

-- 

- Arnaldo