Bug 68597

Summary:

DFG JIT should support integer division

Product:

WebKit

Reporter:

Filip Pizlo <fpizlo>

Component:

JavaScriptCore

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

68582

Attachments:

Description	Flags
the patch	none
the patch	none
the patch - more goodness added	darin: review+

Filip Pizlo

Reported 2011-09-22 00:39:48 PDT

It is entirely sensible to write the following piece of code: a = b / 2; And expect to be able to use 'a' as an integer, if b is always a multiple of 2 and never larger than what an integer can hold. It appears that some Kraken code does this. Currently, the DFG always assumes that all division produces doubles, which results in some bad performance in some programs.

Attachments
the patch (10.04 KB, patch) 2011-09-22 01:01 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch (10.54 KB, patch) 2011-09-22 01:29 PDT, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch - more goodness added (10.67 KB, patch) 2011-09-22 02:08 PDT, Filip Pizlo	darin: review+	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Filip Pizlo

Comment 1 2011-09-22 01:01:45 PDT

Created attachment 108290 [details] the patch

WebKit Review Bot

Comment 2 2011-09-22 01:05:54 PDT

Attachment 108290 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/jit/JITArithmetic.cpp:1054: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.

Filip Pizlo

Comment 3 2011-09-22 01:29:04 PDT

Created attachment 108293 [details] the patch

Filip Pizlo

Comment 4 2011-09-22 02:08:17 PDT

Created attachment 108295 [details] the patch - more goodness added This is a small win by itself, and a big win when combined with https://bugs.webkit.org/show_bug.cgi?id=68582. Benchmark report for SunSpider, V8, and Kraken. VMs tested: "TipOfTree" at /Volumes/Data/pizlo/quinary/OpenSource/WebKitBuild/Release/jsc "IntDiv" at /Volumes/Data/pizlo/OpenSource/WebKitBuild/Release/jsc "IntDivStrCat" at /Volumes/Data/pizlo/septenary/OpenSource/WebKitBuild/Release/jsc Collected 12 samples per benchmark/VM, with 4 VM invocations per benchmark. Used 1 benchmark iteration per VM invocation for warm-up. Used the jsc-specific preciseTime() function to get microsecond-level timing. Reporting benchmark execution times with 95% confidence intervals in milliseconds. TipOfTree IntDiv IntDivStrCat IntDivStrCat v. TipOfTree SunSpider: 3d-cube 7.7031+-0.2217 ? 7.8395+-0.2910 7.6701+-0.1962 3d-morph 7.5458+-0.1330 7.5094+-0.2132 7.4666+-0.2495 might be 1.0106x faster 3d-raytrace 7.5513+-0.2014 ? 7.8452+-0.3016 7.7827+-0.1491 ? might be 1.0306x slower access-binary-trees 2.0995+-0.0579 2.0684+-0.0596 ? 2.1379+-0.1433 ? might be 1.0183x slower access-fannkuch 11.8476+-0.3710 11.7814+-0.2936 ? 11.8475+-0.3008 access-nbody 3.5674+-0.0885 3.4518+-0.0559 ? 3.5763+-0.0967 ? access-nsieve 2.5801+-0.0738 ? 2.6087+-0.0895 2.5845+-0.0629 ? bitops-3bit-bits-in-byte 1.8394+-0.0448 ? 1.8507+-0.0570 1.8055+-0.0425 might be 1.0188x faster bitops-bits-in-byte 2.8397+-0.0606 ? 2.8577+-0.0704 2.8486+-0.0719 ? bitops-bitwise-and 3.7522+-0.1308 3.6579+-0.1029 3.6303+-0.1049 might be 1.0336x faster bitops-nsieve-bits 5.3488+-0.1198 5.2944+-0.1290 ? 5.4255+-0.1257 ? might be 1.0143x slower controlflow-recursive 2.2455+-0.0651 2.2167+-0.0437 2.2005+-0.0542 might be 1.0204x faster crypto-aes 6.8923+-0.3697 6.3176+-0.2402 6.1573+-0.1484 ^ definitely 1.1194x faster crypto-md5 2.8049+-0.0709 ^ 2.5663+-0.0668 2.4669+-0.0729 ^ definitely 1.1370x faster crypto-sha1 2.3478+-0.0652 ^ 2.1351+-0.0607 ? 2.1476+-0.0806 ^ definitely 1.0932x faster date-format-tofte 10.3161+-0.2810 10.2830+-0.2989 ? 10.4249+-0.2628 ? might be 1.0105x slower date-format-xparb 8.7798+-0.2838 8.7494+-0.2496 ? 8.9220+-0.2599 ? might be 1.0162x slower math-cordic 6.3076+-0.1537 6.1111+-0.1149 6.0745+-0.0764 ^ definitely 1.0384x faster math-partial-sums 7.3659+-0.1267 ? 7.4812+-0.1393 ? 7.6897+-0.2075 ? might be 1.0440x slower math-spectral-norm 2.7030+-0.1058 ! 2.9985+-0.0863 ? 3.0546+-0.0794 ! definitely 1.1300x slower regexp-dna 10.8762+-0.1921 10.8190+-0.1663 ? 10.9030+-0.2224 ? string-base64 5.8533+-0.1371 ? 5.8618+-0.1759 ? 5.9906+-0.2396 ? might be 1.0235x slower string-fasta 7.2075+-0.1744 7.1327+-0.1970 7.1087+-0.1609 might be 1.0139x faster string-tagcloud 11.9179+-0.3311 ? 11.9804+-0.3916 ? 12.0016+-0.3266 ? string-unpack-code 18.7440+-0.3804 ? 18.8508+-0.5263 ? 19.2606+-0.6360 ? might be 1.0276x slower string-validate-input 6.4930+-0.0958 ? 6.6237+-0.2460 6.4775+-0.1372 <arithmetic> 6.4435+-0.0377 6.4189+-0.0338 ? 6.4483+-0.0393 ? <geometric> 5.3328+-0.0319 5.2913+-0.0219 ? 5.3018+-0.0402 <harmonic> 4.3898+-0.0353 4.3388+-0.0330 4.3374+-0.0541 might be 1.0121x faster TipOfTree IntDiv IntDivStrCat IntDivStrCat v. TipOfTree V8: crypto 71.2496+-0.2918 ? 71.3862+-0.3429 71.1184+-0.5196 deltablue 237.3015+-1.4425 ? 237.3798+-1.0827 236.5956+-1.3604 earley-boyer 88.2273+-0.3942 88.1260+-0.3059 87.9492+-0.3159 raytrace 64.1057+-0.6186 ? 65.0615+-0.5036 ^ 62.6563+-0.4224 ^ definitely 1.0231x faster regexp 105.5161+-0.4444 ! 106.6996+-0.3355 ! 108.3552+-0.6639 ! definitely 1.0269x slower richards 198.5336+-0.7411 ? 199.2889+-0.7026 199.0919+-0.6748 ? splay 98.3329+-0.2213 ? 98.5661+-0.5356 98.0713+-0.4491 <arithmetic> 123.3238+-0.2493 ? 123.7869+-0.2621 123.4054+-0.1661 ? <geometric> 110.1665+-0.2139 ! 110.6913+-0.2509 ^ 110.1008+-0.1791 <harmonic> 100.1846+-0.2352 ! 100.7525+-0.2618 ^ 99.8989+-0.2373 TipOfTree IntDiv IntDivStrCat IntDivStrCat v. TipOfTree Kraken: ai-astar 616.6539+-2.9734 611.4591+-4.2299 ? 615.3805+-4.1636 audio-beat-detection 467.0274+-4.0021 ^ 377.9344+-1.6609 ^ 204.5658+-1.0722 ^ definitely 2.2830x faster audio-dft 422.0664+-1.7690 421.1265+-2.1315 419.6968+-2.5885 audio-fft 363.5815+-2.0561 ^ 312.9141+-0.8157 ^ 141.5586+-0.9472 ^ definitely 2.5684x faster audio-oscillator 254.2253+-1.3820 ! 258.3617+-1.3677 ? 258.4752+-1.0439 ! definitely 1.0167x slower imaging-darkroom 422.4535+-1.9687 420.4771+-1.1829 ? 421.7214+-0.7025 imaging-desaturate 208.7662+-0.5285 ? 208.9123+-0.7579 ? 210.1087+-1.2589 ? imaging-gaussian-blur 594.3785+-1.2263 594.0332+-0.8333 ! 598.2852+-3.3919 ? json-parse-financial 48.0277+-0.2236 ! 48.6539+-0.3093 48.6490+-0.2884 ! definitely 1.0129x slower json-stringify-tinderbox 68.8110+-0.2238 ^ 68.0999+-0.3462 ! 70.1338+-0.5934 ! definitely 1.0192x slower stanford-crypto-aes 137.5441+-0.4898 ^ 134.0939+-1.0359 ? 135.0720+-0.7700 ^ definitely 1.0183x faster stanford-crypto-ccm 109.4925+-0.5785 ^ 104.6828+-0.9352 ! 105.8698+-0.2324 ^ definitely 1.0342x faster stanford-crypto-pbkdf2 208.6068+-3.2304 204.5755+-0.9357 ? 205.3682+-1.0130 might be 1.0158x faster stanford-crypto-sha256-iterative 84.5903+-0.2556 ? 84.9048+-0.5000 ? 85.2254+-0.4590 ? <arithmetic> 286.1589+-0.5195 ^ 275.0164+-0.4482 ^ 251.4365+-0.4675 ^ definitely 1.1381x faster <geometric> 217.1089+-0.4842 ^ 210.3265+-0.3279 ^ 191.2333+-0.3325 ^ definitely 1.1353x faster <harmonic> 155.4981+-0.3995 ^ 153.0205+-0.3357 ^ 144.4740+-0.3776 ^ definitely 1.0763x faster TipOfTree IntDiv IntDivStrCat IntDivStrCat v. TipOfTree All benchmarks: <arithmetic> 107.1707+-0.1773 ^ 103.9070+-0.1572 ^ 96.8426+-0.1626 ^ definitely 1.1066x faster <geometric> 25.2527+-0.0971 ^ 24.9250+-0.0673 ^ 24.2354+-0.1112 ^ definitely 1.0420x faster <harmonic> 7.7267+-0.0608 7.6381+-0.0568 7.6281+-0.0927 might be 1.0129x faster

Darin Adler

Comment 5 2011-09-22 10:31:50 PDT

Comment on attachment 108295 [details] the patch - more goodness added View in context: https://bugs.webkit.org/attachment.cgi?id=108295&action=review > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:549 > + // have speculatins in place that take care of that separately. We only Typo: speculations > Source/JavaScriptCore/jit/JITArithmetic.cpp:1058 > + // in the heap to go doubly, resulting in double predictions getting predicted to all What’s "go doubly"? Do you just mean "go double"? Or get a double value? > Source/JavaScriptCore/jit/JITArithmetic.cpp:1062 > + // FIXME: This will fail to do its magic if the result is zero. This comment would be clearer if “fail to do its magic” was replaced by a more specific phrase.

Filip Pizlo

Comment 6 2011-09-22 11:44:57 PDT

> > Source/JavaScriptCore/jit/JITArithmetic.cpp:1058 > > + // in the heap to go doubly, resulting in double predictions getting predicted to all > > What’s "go doubly"? Do you just mean "go double"? Or get a double value? I've been using that to distinguish between something having actual honest doubles (i.e. formatted as double because they cannot be represented as integer) and having an odd mix of integers formatted as int32 and integers formatted as double for no reason other than somewhere in the code we were too lazy to realize we were producing integers. > > > Source/JavaScriptCore/jit/JITArithmetic.cpp:1062 > > + // FIXME: This will fail to do its magic if the result is zero. > > This comment would be clearer if “fail to do its magic” was replaced by a more specific phrase. "fail to do its magic" = convert the double back to an integer. I'll fix that comment.

Filip Pizlo

Comment 7 2011-09-22 15:02:16 PDT

Landed in r95754, with changes as per Darin's suggestions.

Note You need to log in before you can comment on or make changes to this bug.