Bug 151607

Summary: Snippefy op_div for the baseline JIT.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, fpizlo, ggaren, keith_miller, msaboff, ossy, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
proposed patch.
none
patch 2: with build fixes for linux ports.
none
patch 3: some cosmetic changes to make later refactoring of other operators to use SnippetOperand cleaner.
ggaren: review+
x86_64 benchmark result.
none
x86 benchmark result.
none
x86_64 benchmark result without DFG.
none
x86 benchmark result without DFG. none

Mark Lam
Reported 2015-11-25 08:58:39 PST
Patch coming.
Attachments
proposed patch. (44.17 KB, patch)
2015-11-25 13:14 PST, Mark Lam 		no flags
DetailsFormatted DiffDiff
patch 2: with build fixes for linux ports. (44.20 KB, patch)
2015-11-26 08:30 PST, Mark Lam 		no flags
DetailsFormatted DiffDiff
patch 3: some cosmetic changes to make later refactoring of other operators to use SnippetOperand cleaner. (44.33 KB, patch)
2015-11-26 11:03 PST, Mark Lam 		ggaren: review+
DetailsFormatted DiffDiff
x86_64 benchmark result. (64.29 KB, text/plain)
2015-11-30 14:03 PST, Mark Lam 		no flags
Details
x86 benchmark result. (64.35 KB, text/plain)
2015-11-30 14:03 PST, Mark Lam 		no flags
Details
x86_64 benchmark result without DFG. (63.53 KB, text/plain)
2015-11-30 14:04 PST, Mark Lam 		no flags
Details
x86 benchmark result without DFG. (63.74 KB, text/plain)
2015-11-30 14:04 PST, Mark Lam 		no flags
Details
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2015-11-25 08:59:30 PST
<rdar://problem/23664944>
Mark Lam
Comment 2 2015-11-25 13:14:05 PST
Created attachment 266168 [details] proposed patch. This patch has passed the JSC tests on x86, x86_64, armv7, and arm64. It has also passed the layout test on x86_64.
WebKit Commit Bot
Comment 3 2015-11-25 13:15:45 PST
Attachment 266168 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:42: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:43: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:44: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:45: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:46: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:47: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:48: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:49: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:50: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:51: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:63: The parameter name "op" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 11 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Csaba Osztrogonác
Comment 4 2015-11-25 15:08:24 PST
I'll check the Linux build in a day.
Csaba Osztrogonác
Comment 5 2015-11-26 03:49:17 PST
Comment on attachment 266168 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=266168&action=review > Source/JavaScriptCore/jit/JITDivGenerator.cpp:29 > +#include "config.h" > + > +#if ENABLE(JIT) > +#include "JITDivGenerator.h" The primary header should be next to config.h, and including JSCJSValueInlines.h fixes EFL build and I think GTK too. #include "config.h" #include "JITDivGenerator.h" #if ENABLE(JIT) #include "JSCJSValueInlines.h"
Mark Lam
Comment 6 2015-11-26 07:02:01 PST
Thanks. Will fix. (In reply to comment #5) > Comment on attachment 266168 [details] > proposed patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=266168&action=review > > > Source/JavaScriptCore/jit/JITDivGenerator.cpp:29 > > +#include "config.h" > > + > > +#if ENABLE(JIT) > > +#include "JITDivGenerator.h" > > The primary header should be next to config.h, and including > JSCJSValueInlines.h fixes EFL build and I think GTK too. > > #include "config.h" > #include "JITDivGenerator.h" > > #if ENABLE(JIT) > > #include "JSCJSValueInlines.h"
Mark Lam
Comment 7 2015-11-26 08:30:03 PST
Created attachment 266187 [details] patch 2: with build fixes for linux ports.
WebKit Commit Bot
Comment 8 2015-11-26 08:32:14 PST
Attachment 266187 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:42: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:43: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:44: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:45: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:46: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:47: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:48: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:49: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:50: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:51: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] Total errors found: 10 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Mark Lam
Comment 9 2015-11-26 11:03:50 PST
Created attachment 266191 [details] patch 3: some cosmetic changes to make later refactoring of other operators to use SnippetOperand cleaner.
WebKit Commit Bot
Comment 10 2015-11-26 11:05:54 PST
Attachment 266191 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:42: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:43: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:44: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:45: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:46: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:47: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:48: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:49: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:50: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITDivGenerator.h:51: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] Total errors found: 10 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Mark Lam
Comment 11 2015-11-30 14:03:08 PST
Created attachment 266274 [details] x86_64 benchmark result.
Mark Lam
Comment 12 2015-11-30 14:03:33 PST
Created attachment 266275 [details] x86 benchmark result.
Mark Lam
Comment 13 2015-11-30 14:04:06 PST
Created attachment 266276 [details] x86_64 benchmark result without DFG.
Mark Lam
Comment 14 2015-11-30 14:04:43 PST
Created attachment 266277 [details] x86 benchmark result without DFG.
Mark Lam
Comment 15 2015-11-30 14:13:08 PST
From the benchmark results, perf appears to be a wash when the DFG is enabled. With the DFG disabled, perf on x86_64 is also a wash. The only "definitely" result that was reproducible was Kraken's audio-fft which was 1.05x slower. However, simply adding 2 xor instructions to the snippet (effectively no-ops) makes this regression disappear. In summary, this regression is likely to be due to cache-line alignment effects i.e. not something we should worry about. With the DFG disabled, perf on x86 is also mostly a wash with 3 differences that were reproducible: LongSpider's math-spectral-norm: definitely 1.0269x slower JSRegress' constant-test: definitely 1.1342x faster JSRegress' div-boolean-double: definitely 1.2549x faster Inspecting the generated code for x86, the op_div snippet is slightly less efficient for x86. This is what accounts for the 2.7x regression on math-spectral-norm. However, this had no impact on the aggregate score for LongSpider. So, we're going to let it slide.
Geoffrey Garen
Comment 16 2015-11-30 14:19:35 PST
> This is what accounts for the 2.7x regression on math-spectral-norm. 2.7%
Mark Lam
Comment 17 2015-11-30 14:21:49 PST
(In reply to comment #16) > > This is what accounts for the 2.7x regression on math-spectral-norm. > > 2.7% Yes, it's a typo: should be 2.7%, not 2.7x.
Geoffrey Garen
Comment 18 2015-11-30 15:06:58 PST
Comment on attachment 266191 [details] patch 3: some cosmetic changes to make later refactoring of other operators to use SnippetOperand cleaner. r=me
Mark Lam
Comment 19 2015-11-30 15:14:32 PST
Thanks for the review. Landed in r192836: <http://trac.webkit.org/r192836>.
Note You need to log in before you can comment on or make changes to this bug.