Bug 227798 - display-profiler-output should be able to print disassembly for the FTL
Summary: display-profiler-output should be able to print disassembly for the FTL
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-07-08 09:08 PDT by Keith Miller
Modified: 2021-07-08 11:54 PDT (History)
6 users (show)

See Also:


Attachments
Patch (16.09 KB, patch)
2021-07-08 09:12 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (24.02 KB, patch)
2021-07-08 10:40 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (24.02 KB, patch)
2021-07-08 10:45 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (22.57 KB, patch)
2021-07-08 10:49 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (14.90 KB, patch)
2021-07-08 11:30 PDT, Keith Miller
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2021-07-08 09:08:59 PDT
display-profiler-output should be able to print disassembly for the FTL
Comment 1 Keith Miller 2021-07-08 09:12:30 PDT
Created attachment 433135 [details]
Patch
Comment 2 Robin Morisset 2021-07-08 10:02:11 PDT
Comment on attachment 433135 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=433135&action=review

> Source/JavaScriptCore/ChangeLog:8
> +        Right now running JSC with the bytecode profiler will will not print

typo: will will

> Source/JavaScriptCore/ChangeLog:11
> +        before the DFG graph despite being printed after before and being added

typo: after before

> Source/JavaScriptCore/ChangeLog:12
> +        to the Profiler::Compilation after the graph. Lastly, the disassmebly

typo: disassmebly

> Source/JavaScriptCore/ChangeLog:13
> +        is the same text as Options::dumpDisassebly so it doesn't include execution

typo: Disassebly
Comment 3 Keith Miller 2021-07-08 10:40:45 PDT
Created attachment 433140 [details]
Patch
Comment 4 Keith Miller 2021-07-08 10:45:07 PDT
Created attachment 433141 [details]
Patch
Comment 5 Keith Miller 2021-07-08 10:49:41 PDT
Created attachment 433142 [details]
Patch
Comment 6 Saam Barati 2021-07-08 11:06:36 PDT
Comment on attachment 433142 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=433142&action=review

> Source/JavaScriptCore/ChangeLog:26
> +        94908                 979/4095/94908/0             Air               Move -824(%rax), %rax, b@402
> +        94908                 979/4095/94908/0             asm                   0x3bf95fa085df: mov -0x338(%rax), %rax
> +        94908                 979/4095/94908/0             b3            Void b@403 = Patchpoint($4396662888(b@90):ColdAny, generator = 0x1066c02e0, earlyClobbered = [], lateClobbered = [], usedRegisters = [%rax, %rbx], resultConstraints = WarmAny, WritesLocalState|Reads:Top, D@65)
> +        94908                 979/4095/94908/0             Air               Patch &Patchpoint0, $0x1060fc068, b@403

It's very deceptive to show DFG node execution counts for B3/Air code. I'd recommend just not showing that info for B3/Air, but still showing it for the parent DFG node.
Comment 7 Saam Barati 2021-07-08 11:07:42 PDT
(In reply to Saam Barati from comment #6)
> Comment on attachment 433142 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=433142&action=review
> 
> > Source/JavaScriptCore/ChangeLog:26
> > +        94908                 979/4095/94908/0             Air               Move -824(%rax), %rax, b@402
> > +        94908                 979/4095/94908/0             asm                   0x3bf95fa085df: mov -0x338(%rax), %rax
> > +        94908                 979/4095/94908/0             b3            Void b@403 = Patchpoint($4396662888(b@90):ColdAny, generator = 0x1066c02e0, earlyClobbered = [], lateClobbered = [], usedRegisters = [%rax, %rbx], resultConstraints = WarmAny, WritesLocalState|Reads:Top, D@65)
> > +        94908                 979/4095/94908/0             Air               Patch &Patchpoint0, $0x1060fc068, b@403
> 
> It's very deceptive to show DFG node execution counts for B3/Air code. I'd
> recommend just not showing that info for B3/Air, but still showing it for
> the parent DFG node.

My reasoning is this:

Many DFG nodes lower to a lot of control flow in B3. Better to not incorrectly say all that control flow executes the same number of times.
Comment 8 Keith Miller 2021-07-08 11:30:43 PDT
Created attachment 433147 [details]
Patch
Comment 9 Keith Miller 2021-07-08 11:54:59 PDT
Landed in r279740.