Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 9 Sep 2021 17:32:56 -0300
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
Message-ID: <YTpveO0qqKFTaxTk@kernel.org>
References: <20210906105640.1040-1-ravi.bangoria@amd.com>
 <20210906105640.1040-2-ravi.bangoria@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20210906105640.1040-2-ravi.bangoria@amd.com>
Precedence: bulk

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