From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Ravi Bangoria <ravi.bangoria@amd.com>
Cc: mark.rutland@arm.com, alexander.shishkin@linux.intel.com,
jolsa@redhat.com, yao.jin@linux.intel.com, namhyung@kernel.org,
kim.phillips@amd.com, linux-perf-users@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] perf annotate: Add fusion logic for AMD microarchs
Date: Thu, 9 Sep 2021 17:32:56 -0300 [thread overview]
Message-ID: <YTpveO0qqKFTaxTk@kernel.org> (raw)
In-Reply-To: <20210906105640.1040-2-ravi.bangoria@amd.com>
Em Mon, Sep 06, 2021 at 04:26:40PM +0530, Ravi Bangoria escreveu:
> AMD family 15h and above microarchs fuse a subset of cmp/test/ALU
> instructions with branch instructions[1][2]. Add perf annotate
> fused instruction support for these microarchs.
>
> Before:
> │ testb $0x80,0x51(%rax)
> │ ┌──jne 5b3
> 0.78 │ │ mov %r13,%rdi
> │ │→ callq mark_page_accessed
> 1.08 │5b3:└─→mov 0x8(%r13),%rax
>
> After:
> │ ┌──testb $0x80,0x51(%rax)
> │ ├──jne 5b3
> 0.78 │ │ mov %r13,%rdi
> │ │→ callq mark_page_accessed
> 1.08 │5b3:└─→mov 0x8(%r13),%rax
>
> [1] https://bugzilla.kernel.org/attachment.cgi?id=298553
> [2] https://bugzilla.kernel.org/attachment.cgi?id=298555
>
> Reported-by: Kim Phillips <kim.phillips@amd.com>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
> ---
> tools/perf/arch/x86/annotate/instructions.c | 37 ++++++++++++++++++++-
> tools/perf/util/annotate.c | 1 +
> 2 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/arch/x86/annotate/instructions.c b/tools/perf/arch/x86/annotate/instructions.c
> index 24ea12ec7e02..46d7124cc4e1 100644
> --- a/tools/perf/arch/x86/annotate/instructions.c
> +++ b/tools/perf/arch/x86/annotate/instructions.c
> @@ -144,8 +144,31 @@ static struct ins x86__instructions[] = {
> { .name = "xorps", .ops = &mov_ops, },
> };
>
> -static bool x86__ins_is_fused(struct arch *arch, const char *ins1,
> +static bool amd__ins_is_fused(struct arch *arch, const char *ins1,
> const char *ins2)
> +{
> + if (strstr(ins2, "jmp"))
> + return false;
> +
> + /* Family >= 15h supports cmp/test + branch fusion */
> + if (arch->family >= 0x15 && (strstarts(ins1, "test") ||
> + (strstarts(ins1, "cmp") && !strstr(ins1, "xchg")))) {
> + return true;
> + }
> +
> + /* Family >= 19h supports some ALU + branch fusion */
> + if (arch->family >= 0x19 && (strstarts(ins1, "add") ||
> + strstarts(ins1, "sub") || strstarts(ins1, "and") ||
> + strstarts(ins1, "inc") || strstarts(ins1, "dec") ||
> + strstarts(ins1, "or") || strstarts(ins1, "xor"))) {
> + return true;
> + }
> +
> + return false;
> +}
> +
> +static bool intel__ins_is_fused(struct arch *arch, const char *ins1,
> + const char *ins2)
> {
> if (arch->family != 6 || arch->model < 0x1e || strstr(ins2, "jmp"))
> return false;
> @@ -172,6 +195,15 @@ static bool x86__ins_is_fused(struct arch *arch, const char *ins1,
> return false;
> }
>
> +static bool x86__ins_is_fused(struct arch *arch, const char *ins1,
> + const char *ins2)
> +{
> + if (strstarts(arch->vendor, "AuthenticAMD"))
> + return amd__ins_is_fused(arch, ins1, ins2);
> +
> + return intel__ins_is_fused(arch, ins1, ins2);
> +}
> +
Can we instead make x86__ins_is_fused be a pointer and instead of
storing arch->vendor we set it to one of amd__ins_is_fused() or
intel__ins_is_fused()?
I.e. here:
> static int x86__cpuid_parse(struct arch *arch, char *cpuid)
> {
> unsigned int family, model, stepping;
> @@ -184,6 +216,9 @@ static int x86__cpuid_parse(struct arch *arch, char *cpuid)
> if (ret == 3) {
> arch->family = family;
> arch->model = model;
> + arch->vendor = strndup(cpuid, 12);
x86__ins_is_fused = strstarts(cpuid, "AuthenticAMD") ?
amd__ins_is_fused :
intel__ins_is_fused;
?
> + if (!arch->vendor)
> + return -1;
> return 0;
> }
>
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 0bae061b2d6d..88326bb990b5 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -77,6 +77,7 @@ struct arch {
> bool sorted_instructions;
> bool initialized;
> void *priv;
> + char *vendor;
> unsigned int model;
> unsigned int family;
> int (*init)(struct arch *arch, char *cpuid);
> --
> 2.27.0
--
- Arnaldo
next prev parent reply other threads:[~2021-09-09 20:33 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-06 10:56 [PATCH 1/2] perf annotate: Fix fused instr logic for assembly functions Ravi Bangoria
2021-09-06 10:56 ` [PATCH 2/2] perf annotate: Add fusion logic for AMD microarchs Ravi Bangoria
2021-09-09 20:32 ` Arnaldo Carvalho de Melo [this message]
2021-09-10 11:17 ` Ravi Bangoria
2021-09-10 14:19 ` Arnaldo Carvalho de Melo
2021-09-11 4:42 ` Ravi Bangoria
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YTpveO0qqKFTaxTk@kernel.org \
--to=acme@kernel.org \
--cc=alexander.shishkin@linux.intel.com \
--cc=jolsa@redhat.com \
--cc=kim.phillips@amd.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=namhyung@kernel.org \
--cc=ravi.bangoria@amd.com \
--cc=yao.jin@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).