From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Jin Yao <yao.jin@linux.intel.com>, Ravi Bangoria <ravi.bangoria@amd.com>
Cc: mark.rutland@arm.com, alexander.shishkin@linux.intel.com,
jolsa@redhat.com, namhyung@kernel.org, kim.phillips@amd.com,
linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/2] perf annotate: Fix fused instr logic for assembly functions
Date: Sat, 11 Sep 2021 16:03:57 -0300 [thread overview]
Message-ID: <YTz9nfyXOSXDdpSE@kernel.org> (raw)
In-Reply-To: <20210911043854.8373-1-ravi.bangoria@amd.com>
Em Sat, Sep 11, 2021 at 10:08:53AM +0530, Ravi Bangoria escreveu:
> Some x86 microarchitectures fuse a subset of cmp/test/ALU instructions
> with branch instructions, and thus perf annotate highlight such valid
> pairs as fused.
Jin, are you ok with this? Can I have your reviewed-by?
- Arnaldo
> When annotated with source, perf uses struct disasm_line to contain
> either source or instruction line from objdump output. Usually, a C
> statement generates multiple instructions which include such
> cmp/test/ALU + branch instruction pairs. But in case of assembly
> function, each individual assembly source line generate one
> instruction. Perf annotate instruction fusion logic assumes previous
> disasm_line as previous instruction line, which is wrong because,
> for assembly function, previous disasm_line contains source line.
> And thus perf fails to highlight valid fused instruction pairs for
> assembly functions.
>
> Fix it by searching backward until we find an instruction line and
> consider that disasm_line as fused with current branch instruction.
>
> Before:
> │ cmpq %rcx, RIP+8(%rsp)
> 0.00 │ cmp %rcx,0x88(%rsp)
> │ je .Lerror_bad_iret <--- Source line
> 0.14 │ ┌──je b4 <--- Instruction line
> │ │movl %ecx, %eax
>
> After:
> │ cmpq %rcx, RIP+8(%rsp)
> 0.00 │ ┌──cmp %rcx,0x88(%rsp)
> │ │je .Lerror_bad_iret
> 0.14 │ ├──je b4
> │ │movl %ecx, %eax
>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com>
> ---
> tools/perf/ui/browser.c | 33 ++++++++++++++++++++++---------
> tools/perf/ui/browser.h | 2 +-
> tools/perf/ui/browsers/annotate.c | 24 +++++++++++++++-------
> 3 files changed, 42 insertions(+), 17 deletions(-)
>
> diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
> index 781afe42e90e..fa5bd5c20e96 100644
> --- a/tools/perf/ui/browser.c
> +++ b/tools/perf/ui/browser.c
> @@ -757,25 +757,40 @@ void __ui_browser__line_arrow(struct ui_browser *browser, unsigned int column,
> }
>
> void ui_browser__mark_fused(struct ui_browser *browser, unsigned int column,
> - unsigned int row, bool arrow_down)
> + unsigned int row, int diff, bool arrow_down)
> {
> - unsigned int end_row;
> + int end_row;
>
> - if (row >= browser->top_idx)
> - end_row = row - browser->top_idx;
> - else
> + if (diff <= 0)
> return;
>
> SLsmg_set_char_set(1);
>
> if (arrow_down) {
> + if (row + diff <= browser->top_idx)
> + return;
> +
> + end_row = row + diff - browser->top_idx;
> ui_browser__gotorc(browser, end_row, column - 1);
> - SLsmg_write_char(SLSMG_ULCORN_CHAR);
> - ui_browser__gotorc(browser, end_row, column);
> - SLsmg_draw_hline(2);
> - ui_browser__gotorc(browser, end_row + 1, column - 1);
> SLsmg_write_char(SLSMG_LTEE_CHAR);
> +
> + while (--end_row >= 0 && end_row > (int)(row - browser->top_idx)) {
> + ui_browser__gotorc(browser, end_row, column - 1);
> + SLsmg_draw_vline(1);
> + }
> +
> + end_row = (int)(row - browser->top_idx);
> + if (end_row >= 0) {
> + ui_browser__gotorc(browser, end_row, column - 1);
> + SLsmg_write_char(SLSMG_ULCORN_CHAR);
> + ui_browser__gotorc(browser, end_row, column);
> + SLsmg_draw_hline(2);
> + }
> } else {
> + if (row < browser->top_idx)
> + return;
> +
> + end_row = row - browser->top_idx;
> ui_browser__gotorc(browser, end_row, column - 1);
> SLsmg_write_char(SLSMG_LTEE_CHAR);
> ui_browser__gotorc(browser, end_row, column);
> diff --git a/tools/perf/ui/browser.h b/tools/perf/ui/browser.h
> index 3678eb88f119..510ce4554050 100644
> --- a/tools/perf/ui/browser.h
> +++ b/tools/perf/ui/browser.h
> @@ -51,7 +51,7 @@ void ui_browser__write_graph(struct ui_browser *browser, int graph);
> void __ui_browser__line_arrow(struct ui_browser *browser, unsigned int column,
> u64 start, u64 end);
> void ui_browser__mark_fused(struct ui_browser *browser, unsigned int column,
> - unsigned int row, bool arrow_down);
> + unsigned int row, int diff, bool arrow_down);
> void __ui_browser__show_title(struct ui_browser *browser, const char *title);
> void ui_browser__show_title(struct ui_browser *browser, const char *title);
> int ui_browser__show(struct ui_browser *browser, const char *title,
> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> index ef4da4295bf7..e81c2493efdf 100644
> --- a/tools/perf/ui/browsers/annotate.c
> +++ b/tools/perf/ui/browsers/annotate.c
> @@ -125,13 +125,20 @@ static void annotate_browser__write(struct ui_browser *browser, void *entry, int
> ab->selection = al;
> }
>
> -static bool is_fused(struct annotate_browser *ab, struct disasm_line *cursor)
> +static int is_fused(struct annotate_browser *ab, struct disasm_line *cursor)
> {
> struct disasm_line *pos = list_prev_entry(cursor, al.node);
> const char *name;
> + int diff = 1;
> +
> + while (pos && pos->al.offset == -1) {
> + pos = list_prev_entry(pos, al.node);
> + if (!ab->opts->hide_src_code)
> + diff++;
> + }
>
> if (!pos)
> - return false;
> + return 0;
>
> if (ins__is_lock(&pos->ins))
> name = pos->ops.locked.ins.name;
> @@ -139,9 +146,11 @@ static bool is_fused(struct annotate_browser *ab, struct disasm_line *cursor)
> name = pos->ins.name;
>
> if (!name || !cursor->ins.name)
> - return false;
> + return 0;
>
> - return ins__is_fused(ab->arch, name, cursor->ins.name);
> + if (ins__is_fused(ab->arch, name, cursor->ins.name))
> + return diff;
> + return 0;
> }
>
> static void annotate_browser__draw_current_jump(struct ui_browser *browser)
> @@ -155,6 +164,7 @@ static void annotate_browser__draw_current_jump(struct ui_browser *browser)
> struct annotation *notes = symbol__annotation(sym);
> u8 pcnt_width = annotation__pcnt_width(notes);
> int width;
> + int diff = 0;
>
> /* PLT symbols contain external offsets */
> if (strstr(sym->name, "@plt"))
> @@ -205,11 +215,11 @@ static void annotate_browser__draw_current_jump(struct ui_browser *browser)
> pcnt_width + 2 + notes->widths.addr + width,
> from, to);
>
> - if (is_fused(ab, cursor)) {
> + diff = is_fused(ab, cursor);
> + if (diff > 0) {
> ui_browser__mark_fused(browser,
> pcnt_width + 3 + notes->widths.addr + width,
> - from - 1,
> - to > from);
> + from - diff, diff, to > from);
> }
> }
>
> --
> 2.27.0
--
- Arnaldo
next prev parent reply other threads:[~2021-09-11 19:04 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-11 4:38 [PATCH v2 1/2] perf annotate: Fix fused instr logic for assembly functions Ravi Bangoria
2021-09-11 4:38 ` [PATCH v2 2/2] perf annotate: Add fusion logic for AMD microarchs Ravi Bangoria
2021-09-11 19:03 ` Arnaldo Carvalho de Melo [this message]
2021-09-13 1:54 ` [PATCH v2 1/2] perf annotate: Fix fused instr logic for assembly functions Jin, Yao
2021-09-13 19:53 ` Arnaldo Carvalho de Melo
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=YTz9nfyXOSXDdpSE@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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.