diff mbox series

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

Message ID 20230524205054.3087004-1-namhyung@kernel.org
State New
Headers
Series [v2,1/2] perf annotate: Handle x86 instruction suffix generally |

Commit Message

Namhyung Kim May 24, 2023, 8:50 p.m. UTC 
  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(+)

Comments

Adrian Hunter May 25, 2023, 5:21 a.m. UTC | #1 
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;
>  }
>
Namhyung Kim June 5, 2023, 11:56 p.m. UTC | #2 
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;
> >  }
> >
>
Masami Hiramatsu (Google) June 6, 2023, 2:06 p.m. UTC | #3 
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
>
Arnaldo Carvalho de Melo June 6, 2023, 6 p.m. UTC | #4 
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>
diff mbox series

Patch

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;
 }