perf annotate: Add more x86 mov instruction cases

[PATCH] perf annotate: Add more x86 mov instruction cases

Posted by Namhyung Kim 2 years, 3 months ago

Instructions with sign- and zero- extention like movsbl and movzwq were
not handled properly.  As it can check different size suffix (-b, -w, -l
or -q) we can omit that and add the common parts even though some
combinations are not possible.

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

diff --git a/tools/perf/arch/x86/annotate/instructions.c b/tools/perf/arch/x86/annotate/instructions.c
index 5f4ac4fc7fcf..5cdf457f5cbe 100644
--- a/tools/perf/arch/x86/annotate/instructions.c
+++ b/tools/perf/arch/x86/annotate/instructions.c
@@ -74,12 +74,15 @@ static struct ins x86__instructions[] = {
 	{ .name = "movdqa",	.ops = &mov_ops,  },
 	{ .name = "movdqu",	.ops = &mov_ops,  },
 	{ .name = "movsd",	.ops = &mov_ops,  },
-	{ .name = "movslq",	.ops = &mov_ops,  },
 	{ .name = "movss",	.ops = &mov_ops,  },
+	{ .name = "movsb",	.ops = &mov_ops,  },
+	{ .name = "movsw",	.ops = &mov_ops,  },
+	{ .name = "movsl",	.ops = &mov_ops,  },
 	{ .name = "movupd",	.ops = &mov_ops,  },
 	{ .name = "movups",	.ops = &mov_ops,  },
-	{ .name = "movzbl",	.ops = &mov_ops,  },
-	{ .name = "movzwl",	.ops = &mov_ops,  },
+	{ .name = "movzb",	.ops = &mov_ops,  },
+	{ .name = "movzw",	.ops = &mov_ops,  },
+	{ .name = "movzl",	.ops = &mov_ops,  },
 	{ .name = "mulsd",	.ops = &mov_ops,  },
 	{ .name = "mulss",	.ops = &mov_ops,  },
 	{ .name = "nop",	.ops = &nop_ops,  },
-- 
2.42.0.283.g2d96d420d3-goog

Re: [PATCH] perf annotate: Add more x86 mov instruction cases

Posted by Ian Rogers 2 years, 3 months ago

On Thu, Sep 7, 2023 at 10:22 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Instructions with sign- and zero- extention like movsbl and movzwq were
> not handled properly.  As it can check different size suffix (-b, -w, -l
> or -q) we can omit that and add the common parts even though some
> combinations are not possible.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/arch/x86/annotate/instructions.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/arch/x86/annotate/instructions.c b/tools/perf/arch/x86/annotate/instructions.c
> index 5f4ac4fc7fcf..5cdf457f5cbe 100644
> --- a/tools/perf/arch/x86/annotate/instructions.c
> +++ b/tools/perf/arch/x86/annotate/instructions.c
> @@ -74,12 +74,15 @@ static struct ins x86__instructions[] = {
>         { .name = "movdqa",     .ops = &mov_ops,  },
>         { .name = "movdqu",     .ops = &mov_ops,  },
>         { .name = "movsd",      .ops = &mov_ops,  },
> -       { .name = "movslq",     .ops = &mov_ops,  },
>         { .name = "movss",      .ops = &mov_ops,  },
> +       { .name = "movsb",      .ops = &mov_ops,  },
> +       { .name = "movsw",      .ops = &mov_ops,  },
> +       { .name = "movsl",      .ops = &mov_ops,  },

In Intel's manual some of these names are "Move Data From String to
String" operations, movsb and movsw in particular. These instructions
can be used to make simple memcpy loops. Could it be the past omission
was deliberate due to the different way the addressing works in the
instructions?

Thanks,
Ian

>         { .name = "movupd",     .ops = &mov_ops,  },
>         { .name = "movups",     .ops = &mov_ops,  },
> -       { .name = "movzbl",     .ops = &mov_ops,  },
> -       { .name = "movzwl",     .ops = &mov_ops,  },
> +       { .name = "movzb",      .ops = &mov_ops,  },
> +       { .name = "movzw",      .ops = &mov_ops,  },
> +       { .name = "movzl",      .ops = &mov_ops,  },
>         { .name = "mulsd",      .ops = &mov_ops,  },
>         { .name = "mulss",      .ops = &mov_ops,  },
>         { .name = "nop",        .ops = &nop_ops,  },
> --
> 2.42.0.283.g2d96d420d3-goog
>

Re: [PATCH] perf annotate: Add more x86 mov instruction cases

Posted by Namhyung Kim 2 years, 3 months ago

Hi Ian,

On Thu, Sep 7, 2023 at 11:24 PM Ian Rogers <irogers@google.com> wrote:
>
> On Thu, Sep 7, 2023 at 10:22 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > Instructions with sign- and zero- extention like movsbl and movzwq were
> > not handled properly.  As it can check different size suffix (-b, -w, -l
> > or -q) we can omit that and add the common parts even though some
> > combinations are not possible.
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/arch/x86/annotate/instructions.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/perf/arch/x86/annotate/instructions.c b/tools/perf/arch/x86/annotate/instructions.c
> > index 5f4ac4fc7fcf..5cdf457f5cbe 100644
> > --- a/tools/perf/arch/x86/annotate/instructions.c
> > +++ b/tools/perf/arch/x86/annotate/instructions.c
> > @@ -74,12 +74,15 @@ static struct ins x86__instructions[] = {
> >         { .name = "movdqa",     .ops = &mov_ops,  },
> >         { .name = "movdqu",     .ops = &mov_ops,  },
> >         { .name = "movsd",      .ops = &mov_ops,  },
> > -       { .name = "movslq",     .ops = &mov_ops,  },
> >         { .name = "movss",      .ops = &mov_ops,  },
> > +       { .name = "movsb",      .ops = &mov_ops,  },
> > +       { .name = "movsw",      .ops = &mov_ops,  },
> > +       { .name = "movsl",      .ops = &mov_ops,  },
>
> In Intel's manual some of these names are "Move Data From String to
> String" operations, movsb and movsw in particular. These instructions
> can be used to make simple memcpy loops. Could it be the past omission
> was deliberate due to the different way the addressing works in the
> instructions?

I don't know but in terms of instruction parsing, they are the same
"MOVE" with two operands.  I'm not aware of anything in perf with
the operands of these instructions.  So I guess it'd be fine to add
these instructions even if they have different underlying behaviors.

Thanks,
Namhyung

RE: [PATCH] perf annotate: Add more x86 mov instruction cases

Posted by David Laight 2 years, 3 months ago

From: Namhyung Kim
> Sent: 09 September 2023 00:56
> 
> Hi Ian,
> 
> On Thu, Sep 7, 2023 at 11:24 PM Ian Rogers <irogers@google.com> wrote:
> >
> > On Thu, Sep 7, 2023 at 10:22 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > >
> > > Instructions with sign- and zero- extention like movsbl and movzwq were
> > > not handled properly.  As it can check different size suffix (-b, -w, -l
> > > or -q) we can omit that and add the common parts even though some
> > > combinations are not possible.
> > >
> > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > > ---
> > >  tools/perf/arch/x86/annotate/instructions.c | 9 ++++++---
> > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/tools/perf/arch/x86/annotate/instructions.c
> b/tools/perf/arch/x86/annotate/instructions.c
> > > index 5f4ac4fc7fcf..5cdf457f5cbe 100644
> > > --- a/tools/perf/arch/x86/annotate/instructions.c
> > > +++ b/tools/perf/arch/x86/annotate/instructions.c
> > > @@ -74,12 +74,15 @@ static struct ins x86__instructions[] = {
> > >         { .name = "movdqa",     .ops = &mov_ops,  },
> > >         { .name = "movdqu",     .ops = &mov_ops,  },
> > >         { .name = "movsd",      .ops = &mov_ops,  },
> > > -       { .name = "movslq",     .ops = &mov_ops,  },
> > >         { .name = "movss",      .ops = &mov_ops,  },
> > > +       { .name = "movsb",      .ops = &mov_ops,  },
> > > +       { .name = "movsw",      .ops = &mov_ops,  },
> > > +       { .name = "movsl",      .ops = &mov_ops,  },
> >
> > In Intel's manual some of these names are "Move Data From String to
> > String" operations, movsb and movsw in particular. These instructions
> > can be used to make simple memcpy loops. Could it be the past omission
> > was deliberate due to the different way the addressing works in the
> > instructions?
> 
> I don't know but in terms of instruction parsing, they are the same
> "MOVE" with two operands.  I'm not aware of anything in perf with
> the operands of these instructions.  So I guess it'd be fine to add
> these instructions even if they have different underlying behaviors.

I'm pretty sure that 'rep movs[bwlq]' (aka while (cx--) *di++ = *si++)
is likely to be missing the memory argument parameters.
There is also 'fun and games' with one variant - iirc 'rep movsd'
what has been used for 64bit, but got hijacked by one of the SIMD sets.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] perf annotate: Add more x86 mov instruction cases

Posted by Namhyung Kim 2 years, 3 months ago

On Mon, Sep 11, 2023 at 1:12 AM David Laight <David.Laight@aculab.com> wrote:
>
> From: Namhyung Kim
> > Sent: 09 September 2023 00:56
> >
> > Hi Ian,
> >
> > On Thu, Sep 7, 2023 at 11:24 PM Ian Rogers <irogers@google.com> wrote:
> > >
> > > On Thu, Sep 7, 2023 at 10:22 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > > >
> > > > Instructions with sign- and zero- extention like movsbl and movzwq were
> > > > not handled properly.  As it can check different size suffix (-b, -w, -l
> > > > or -q) we can omit that and add the common parts even though some
> > > > combinations are not possible.
> > > >
> > > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > > > ---
> > > >  tools/perf/arch/x86/annotate/instructions.c | 9 ++++++---
> > > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/tools/perf/arch/x86/annotate/instructions.c
> > b/tools/perf/arch/x86/annotate/instructions.c
> > > > index 5f4ac4fc7fcf..5cdf457f5cbe 100644
> > > > --- a/tools/perf/arch/x86/annotate/instructions.c
> > > > +++ b/tools/perf/arch/x86/annotate/instructions.c
> > > > @@ -74,12 +74,15 @@ static struct ins x86__instructions[] = {
> > > >         { .name = "movdqa",     .ops = &mov_ops,  },
> > > >         { .name = "movdqu",     .ops = &mov_ops,  },
> > > >         { .name = "movsd",      .ops = &mov_ops,  },
> > > > -       { .name = "movslq",     .ops = &mov_ops,  },
> > > >         { .name = "movss",      .ops = &mov_ops,  },
> > > > +       { .name = "movsb",      .ops = &mov_ops,  },
> > > > +       { .name = "movsw",      .ops = &mov_ops,  },
> > > > +       { .name = "movsl",      .ops = &mov_ops,  },
> > >
> > > In Intel's manual some of these names are "Move Data From String to
> > > String" operations, movsb and movsw in particular. These instructions
> > > can be used to make simple memcpy loops. Could it be the past omission
> > > was deliberate due to the different way the addressing works in the
> > > instructions?
> >
> > I don't know but in terms of instruction parsing, they are the same
> > "MOVE" with two operands.  I'm not aware of anything in perf with
> > the operands of these instructions.  So I guess it'd be fine to add
> > these instructions even if they have different underlying behaviors.
>
> I'm pretty sure that 'rep movs[bwlq]' (aka while (cx--) *di++ = *si++)
> is likely to be missing the memory argument parameters.
> There is also 'fun and games' with one variant - iirc 'rep movsd'
> what has been used for 64bit, but got hijacked by one of the SIMD sets.

It seems perf annotate don't process the rep prefix yet.
So I think there should be no functional change now.

Thanks,
Namhyung

Re: [PATCH] perf annotate: Add more x86 mov instruction cases

Posted by Ian Rogers 2 years, 3 months ago

On Wed, Sep 13, 2023 at 2:14 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Mon, Sep 11, 2023 at 1:12 AM David Laight <David.Laight@aculab.com> wrote:
> >
> > From: Namhyung Kim
> > > Sent: 09 September 2023 00:56
> > >
> > > Hi Ian,
> > >
> > > On Thu, Sep 7, 2023 at 11:24 PM Ian Rogers <irogers@google.com> wrote:
> > > >
> > > > On Thu, Sep 7, 2023 at 10:22 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > > > >
> > > > > Instructions with sign- and zero- extention like movsbl and movzwq were
> > > > > not handled properly.  As it can check different size suffix (-b, -w, -l
> > > > > or -q) we can omit that and add the common parts even though some
> > > > > combinations are not possible.
> > > > >
> > > > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > > > > ---
> > > > >  tools/perf/arch/x86/annotate/instructions.c | 9 ++++++---
> > > > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/tools/perf/arch/x86/annotate/instructions.c
> > > b/tools/perf/arch/x86/annotate/instructions.c
> > > > > index 5f4ac4fc7fcf..5cdf457f5cbe 100644
> > > > > --- a/tools/perf/arch/x86/annotate/instructions.c
> > > > > +++ b/tools/perf/arch/x86/annotate/instructions.c
> > > > > @@ -74,12 +74,15 @@ static struct ins x86__instructions[] = {
> > > > >         { .name = "movdqa",     .ops = &mov_ops,  },
> > > > >         { .name = "movdqu",     .ops = &mov_ops,  },
> > > > >         { .name = "movsd",      .ops = &mov_ops,  },
> > > > > -       { .name = "movslq",     .ops = &mov_ops,  },
> > > > >         { .name = "movss",      .ops = &mov_ops,  },
> > > > > +       { .name = "movsb",      .ops = &mov_ops,  },
> > > > > +       { .name = "movsw",      .ops = &mov_ops,  },
> > > > > +       { .name = "movsl",      .ops = &mov_ops,  },
> > > >
> > > > In Intel's manual some of these names are "Move Data From String to
> > > > String" operations, movsb and movsw in particular. These instructions
> > > > can be used to make simple memcpy loops. Could it be the past omission
> > > > was deliberate due to the different way the addressing works in the
> > > > instructions?
> > >
> > > I don't know but in terms of instruction parsing, they are the same
> > > "MOVE" with two operands.  I'm not aware of anything in perf with
> > > the operands of these instructions.  So I guess it'd be fine to add
> > > these instructions even if they have different underlying behaviors.
> >
> > I'm pretty sure that 'rep movs[bwlq]' (aka while (cx--) *di++ = *si++)
> > is likely to be missing the memory argument parameters.
> > There is also 'fun and games' with one variant - iirc 'rep movsd'
> > what has been used for 64bit, but got hijacked by one of the SIMD sets.
>
> It seems perf annotate don't process the rep prefix yet.
> So I think there should be no functional change now.

Reading the code, I also think it should be okay. Doing:
```
$ objdump -d /usr/lib/x86_64-linux-gnu/libc.so.6 |grep rep
   2650a:       f3 ab                   rep stos %eax,%es:(%rdi)
   33b76:       f3 48 a5                rep movsq %ds:(%rsi),%es:(%rdi)
...
```
The mov parsing logic doesn't appear to care about src and dest, and
the formatting above matches other mov cases.

Reviewed-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> Thanks,
> Namhyung

Re: [PATCH] perf annotate: Add more x86 mov instruction cases

Posted by Namhyung Kim 2 years, 3 months ago

On Wed, Sep 13, 2023 at 4:24 PM Ian Rogers <irogers@google.com> wrote:
>
> On Wed, Sep 13, 2023 at 2:14 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Mon, Sep 11, 2023 at 1:12 AM David Laight <David.Laight@aculab.com> wrote:
> > >
> > > From: Namhyung Kim
> > > > Sent: 09 September 2023 00:56
> > > >
> > > > Hi Ian,
> > > >
> > > > On Thu, Sep 7, 2023 at 11:24 PM Ian Rogers <irogers@google.com> wrote:
> > > > >
> > > > > On Thu, Sep 7, 2023 at 10:22 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > > > > >
> > > > > > Instructions with sign- and zero- extention like movsbl and movzwq were
> > > > > > not handled properly.  As it can check different size suffix (-b, -w, -l
> > > > > > or -q) we can omit that and add the common parts even though some
> > > > > > combinations are not possible.
> > > > > >
> > > > > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > > > > > ---
> > > > > >  tools/perf/arch/x86/annotate/instructions.c | 9 ++++++---
> > > > > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/tools/perf/arch/x86/annotate/instructions.c
> > > > b/tools/perf/arch/x86/annotate/instructions.c
> > > > > > index 5f4ac4fc7fcf..5cdf457f5cbe 100644
> > > > > > --- a/tools/perf/arch/x86/annotate/instructions.c
> > > > > > +++ b/tools/perf/arch/x86/annotate/instructions.c
> > > > > > @@ -74,12 +74,15 @@ static struct ins x86__instructions[] = {
> > > > > >         { .name = "movdqa",     .ops = &mov_ops,  },
> > > > > >         { .name = "movdqu",     .ops = &mov_ops,  },
> > > > > >         { .name = "movsd",      .ops = &mov_ops,  },
> > > > > > -       { .name = "movslq",     .ops = &mov_ops,  },
> > > > > >         { .name = "movss",      .ops = &mov_ops,  },
> > > > > > +       { .name = "movsb",      .ops = &mov_ops,  },
> > > > > > +       { .name = "movsw",      .ops = &mov_ops,  },
> > > > > > +       { .name = "movsl",      .ops = &mov_ops,  },
> > > > >
> > > > > In Intel's manual some of these names are "Move Data From String to
> > > > > String" operations, movsb and movsw in particular. These instructions
> > > > > can be used to make simple memcpy loops. Could it be the past omission
> > > > > was deliberate due to the different way the addressing works in the
> > > > > instructions?
> > > >
> > > > I don't know but in terms of instruction parsing, they are the same
> > > > "MOVE" with two operands.  I'm not aware of anything in perf with
> > > > the operands of these instructions.  So I guess it'd be fine to add
> > > > these instructions even if they have different underlying behaviors.
> > >
> > > I'm pretty sure that 'rep movs[bwlq]' (aka while (cx--) *di++ = *si++)
> > > is likely to be missing the memory argument parameters.
> > > There is also 'fun and games' with one variant - iirc 'rep movsd'
> > > what has been used for 64bit, but got hijacked by one of the SIMD sets.
> >
> > It seems perf annotate don't process the rep prefix yet.
> > So I think there should be no functional change now.
>
> Reading the code, I also think it should be okay. Doing:
> ```
> $ objdump -d /usr/lib/x86_64-linux-gnu/libc.so.6 |grep rep
>    2650a:       f3 ab                   rep stos %eax,%es:(%rdi)
>    33b76:       f3 48 a5                rep movsq %ds:(%rsi),%es:(%rdi)
> ...
> ```
> The mov parsing logic doesn't appear to care about src and dest, and
> the formatting above matches other mov cases.
>
> Reviewed-by: Ian Rogers <irogers@google.com>

Applied to perf-tools-next, thanks!

Namhyung