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

Description Mark Lam 2015-11-25 08:58:39 PST 
Patch coming.
Comment 1 Radar WebKit Bug Importer 2015-11-25 08:59:30 PST 
<rdar://problem/23664944>
Comment 2 Mark Lam 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.
Comment 3 WebKit Commit Bot 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.
Comment 4 Csaba Osztrogonác 2015-11-25 15:08:24 PST 
I'll check the Linux build in a day.
Comment 5 Csaba Osztrogonác 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"
Comment 6 Mark Lam 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"
Comment 7 Mark Lam 2015-11-26 08:30:03 PST 
Created attachment 266187 [details]
patch 2: with build fixes for linux ports.
Comment 8 WebKit Commit Bot 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.
Comment 9 Mark Lam 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.
Comment 10 WebKit Commit Bot 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.
Comment 11 Mark Lam 2015-11-30 14:03:08 PST 
Created attachment 266274 [details]
x86_64 benchmark result.
Comment 12 Mark Lam 2015-11-30 14:03:33 PST 
Created attachment 266275 [details]
x86 benchmark result.
Comment 13 Mark Lam 2015-11-30 14:04:06 PST 
Created attachment 266276 [details]
x86_64 benchmark result without DFG.
Comment 14 Mark Lam 2015-11-30 14:04:43 PST 
Created attachment 266277 [details]
x86 benchmark result without DFG.
Comment 15 Mark Lam 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.
Comment 16 Geoffrey Garen 2015-11-30 14:19:35 PST 
> This is what accounts for the 2.7x regression on math-spectral-norm.

2.7%
Comment 17 Mark Lam 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.
Comment 18 Geoffrey Garen 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
Comment 19 Mark Lam 2015-11-30 15:14:32 PST 
Thanks for the review.  Landed in r192836: <http://trac.webkit.org/r192836>.