Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1682913 > unrolled thread
| Started by | Jin Yao <yao.jin@linux.intel.com> |
|---|---|
| First post | 2017-07-07 07:10 +0200 |
| Last post | 2017-07-10 02:40 +0200 |
| Articles | 3 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[PATCH v4 2/2] perf report: Implement visual marker for macro fusion in annotate Jin Yao <yao.jin@linux.intel.com> - 2017-07-07 07:10 +0200
Re: [PATCH v4 2/2] perf report: Implement visual marker for macro fusion in annotate Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-07-07 17:00 +0200
Re: [PATCH v4 2/2] perf report: Implement visual marker for macro fusion in annotate "Jin, Yao" <yao.jin@linux.intel.com> - 2017-07-10 02:40 +0200
| From | Jin Yao <yao.jin@linux.intel.com> |
|---|---|
| Date | 2017-07-07 07:10 +0200 |
| Subject | [PATCH v4 2/2] perf report: Implement visual marker for macro fusion in annotate |
| Message-ID | <u0tGF-7Fp-5@gated-at.bofh.it> |
For marking the fused instructions clearly, This patch adds a
line before the first instruction of pair and joins it with the
arrow of the jump.
For example, when je is selected in annotate view, the line
before cmpl is displayed and joins the arrow of je.
│ ┌──cmpl $0x0,argp_program_version_hook
81.93 │ ├──je 20
│ │ lock cmpxchg %esi,0x38a9a4(%rip)
│ │↓ jne 29
│ │↓ jmp 43
11.47 │20:└─→cmpxch %esi,0x38a999(%rip)
That means the cmpl+je is fused instruction pair and they should
be considered together.
Change-log:
-----------
v4: Since the first patch in patch set is changed, this is mainly
changed for compilation.
v3: Use Arnaldo's fix to let the display be better.
To get the evsel->evlist->env->cpuid, save the evsel in
annotate_browser.
v2: No more changes, just uses a new function "ins__is_fused"
to check if the instructions are fused.
v1: Initial post
Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
tools/perf/ui/browser.c | 29 +++++++++++++++++++++++++++++
tools/perf/ui/browser.h | 2 ++
tools/perf/ui/browsers/annotate.c | 26 ++++++++++++++++++++++++++
tools/perf/util/annotate.c | 5 +++++
tools/perf/util/annotate.h | 1 +
5 files changed, 63 insertions(+)
diff --git a/tools/perf/ui/browser.c b/tools/perf/ui/browser.c
index a4d3762..9ef7677 100644
--- a/tools/perf/ui/browser.c
+++ b/tools/perf/ui/browser.c
@@ -738,6 +738,35 @@ void __ui_browser__line_arrow(struct ui_browser *browser, unsigned int column,
__ui_browser__line_arrow_down(browser, column, start, end);
}
+void ui_browser__mark_fused(struct ui_browser *browser, unsigned int column,
+ unsigned int row, bool arrow_down)
+{
+ unsigned int end_row;
+
+ if (row >= browser->top_idx)
+ end_row = row - browser->top_idx;
+ else
+ return;
+
+ SLsmg_set_char_set(1);
+
+ if (arrow_down) {
+ 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);
+ } else {
+ ui_browser__gotorc(browser, end_row, column - 1);
+ SLsmg_write_char(SLSMG_LTEE_CHAR);
+ ui_browser__gotorc(browser, end_row, column);
+ SLsmg_draw_hline(2);
+ }
+
+ SLsmg_set_char_set(0);
+}
+
void ui_browser__init(void)
{
int i = 0;
diff --git a/tools/perf/ui/browser.h b/tools/perf/ui/browser.h
index be3b70e..a12eff7 100644
--- a/tools/perf/ui/browser.h
+++ b/tools/perf/ui/browser.h
@@ -43,6 +43,8 @@ void ui_browser__printf(struct ui_browser *browser, const char *fmt, ...);
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);
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 ae05ebb..b376637 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -273,6 +273,25 @@ static bool disasm_line__is_valid_jump(struct disasm_line *dl, struct symbol *sy
return true;
}
+static bool is_fused(struct annotate_browser *ab, struct disasm_line *cursor)
+{
+ struct disasm_line *pos = list_prev_entry(cursor, node);
+ const char *name;
+
+ if (!pos)
+ return false;
+
+ if (ins__is_lock(&pos->ins))
+ name = pos->ops.locked.ins.name;
+ else
+ name = pos->ins.name;
+
+ if (!name || !cursor->ins.name)
+ return false;
+
+ return ins__is_fused(ab->arch, name, cursor->ins.name);
+}
+
static void annotate_browser__draw_current_jump(struct ui_browser *browser)
{
struct annotate_browser *ab = container_of(browser, struct annotate_browser, b);
@@ -308,6 +327,13 @@ static void annotate_browser__draw_current_jump(struct ui_browser *browser)
ui_browser__set_color(browser, HE_COLORSET_JUMP_ARROWS);
__ui_browser__line_arrow(browser, pcnt_width + 2 + ab->addr_width,
from, to);
+
+ if (is_fused(ab, cursor)) {
+ ui_browser__mark_fused(browser,
+ pcnt_width + 3 + ab->addr_width,
+ from - 1,
+ to > from ? true : false);
+ }
}
static unsigned int annotate_browser__refresh(struct ui_browser *browser)
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 8748ebb..ef434b5 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -517,6 +517,11 @@ bool ins__is_ret(const struct ins *ins)
return ins->ops == &ret_ops;
}
+bool ins__is_lock(const struct ins *ins)
+{
+ return ins->ops == &lock_ops;
+}
+
static int ins__key_cmp(const void *name, const void *insp)
{
const struct ins *ins = insp;
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 72d7272..bac698d 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -52,6 +52,7 @@ struct ins_ops {
bool ins__is_jump(const struct ins *ins);
bool ins__is_call(const struct ins *ins);
bool ins__is_ret(const struct ins *ins);
+bool ins__is_lock(const struct ins *ins);
int ins__scnprintf(struct ins *ins, char *bf, size_t size, struct ins_operands *ops);
bool ins__is_fused(struct arch *arch, const char *ins1, const char *ins2);
--
2.7.4
[toc] | [next] | [standalone]
| From | Arnaldo Carvalho de Melo <acme@kernel.org> |
|---|---|
| Date | 2017-07-07 17:00 +0200 |
| Subject | Re: [PATCH v4 2/2] perf report: Implement visual marker for macro fusion in annotate |
| Message-ID | <u0CTF-5qc-31@gated-at.bofh.it> |
| In reply to | #1682913 |
Em Fri, Jul 07, 2017 at 01:06:35PM +0800, Jin Yao escreveu:
> For marking the fused instructions clearly, This patch adds a line
> before the first instruction of pair and joins it with the arrow of the
> jump.
>
> For example, when je is selected in annotate view, the line before cmpl
> is displayed and joins the arrow of je.
>
> │ ┌──cmpl $0x0,argp_program_version_hook
> 81.93 │ ├──je 20
> │ │ lock cmpxchg %esi,0x38a9a4(%rip)
> │ │↓ jne 29
> │ │↓ jmp 43
> 11.47 │20:└─→cmpxch %esi,0x38a999(%rip)
>
> That means the cmpl+je is fused instruction pair and they should be
> considered together.
I applied this one, no unnecessary parsing of cpuid done at each
jump->target arrow rendering, much better, thanks!
One thing for a follow up patch:
We have this when the cursor is at a jump instruction:
│ ┌──test %ecx,%ecx
-> │ ├──je 714cf
│ │ mov LINES+0xb40,%edx
│ │ test %edx,%edx
│ │↓ je 71580
│714cf:└─→mov LINES+0x10c8,%eax
But if we go up a line, to that "test" instruction, we get:
-> │ test %ecx,%ecx
│ ↓ je 714cf
│ mov LINES+0xb40,%edx
│ test %edx,%edx
│ ↓ je 71580
│714cf: mov LINES+0x10c8,%eax
I suggest that this be changed to:
-> │ ┌─test %ecx,%ecx
│ ↓ je 714cf
│ mov LINES+0xb40,%edx
│ test %edx,%edx
│ ↓ je 71580
│714cf: mov LINES+0x10c8,%eax
I.e. even before going to the jump instruction line with the cursor, we
would see the fused instructions.
To do that perhaps we should improve annotate_browser__draw_current_jump
to improve that part that looks for is_valid_jump() to consider
instructions that could be fused with jumps for the machine where the
perf data came from, etc.
But the current situation is better already, thanks for your work,
applied!
- Arnaldo
[toc] | [prev] | [next] | [standalone]
| From | "Jin, Yao" <yao.jin@linux.intel.com> |
|---|---|
| Date | 2017-07-10 02:40 +0200 |
| Subject | Re: [PATCH v4 2/2] perf report: Implement visual marker for macro fusion in annotate |
| Message-ID | <u1uU1-6gW-1@gated-at.bofh.it> |
| In reply to | #1683236 |
> I applied this one, no unnecessary parsing of cpuid done at each > jump->target arrow rendering, much better, thanks! > > One thing for a follow up patch: > > We have this when the cursor is at a jump instruction: > > │ ┌──test %ecx,%ecx > -> │ ├──je 714cf > │ │ mov LINES+0xb40,%edx > │ │ test %edx,%edx > │ │↓ je 71580 > │714cf:└─→mov LINES+0x10c8,%eax > > But if we go up a line, to that "test" instruction, we get: > > -> │ test %ecx,%ecx > │ ↓ je 714cf > │ mov LINES+0xb40,%edx > │ test %edx,%edx > │ ↓ je 71580 > │714cf: mov LINES+0x10c8,%eax > > I suggest that this be changed to: > > -> │ ┌─test %ecx,%ecx > │ ↓ je 714cf > │ mov LINES+0xb40,%edx > │ test %edx,%edx > │ ↓ je 71580 > │714cf: mov LINES+0x10c8,%eax > > I.e. even before going to the jump instruction line with the cursor, we > would see the fused instructions. > > To do that perhaps we should improve annotate_browser__draw_current_jump > to improve that part that looks for is_valid_jump() to consider > instructions that could be fused with jumps for the machine where the > perf data came from, etc. > > But the current situation is better already, thanks for your work, > applied! > > - Arnaldo I will investigate how to do the follow-up patch. Thanks Jin Yao
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web