Summary: | Enable the DFG JIT on x86-64 Linux platforms | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andy Wingo <wingo> | ||||||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | barraclough, fpizlo, hausmann, ossy, webkit.review.bot, yuqiang.xian, zherczeg | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | 71882 | ||||||||||||||
Bug Blocks: | |||||||||||||||
Attachments: |
|
Description
Andy Wingo
2011-11-02 09:45:11 PDT
Yes! - we want to turn this on as soon as there are no known regressions, and I think we're pretty close to being there. Last time I tested it looked like there were two sets of layout tests still being broken by the DFG JIT on x86 - the jquery & inspector tests. I'm pretty sure that Yuqiang's patch to change how we set boolean results should have fixed the jquery regressions, the inspector ones might also have been fixed since I last tried them, too. There is no reason I know of for it not to be enabled on x86-64 non-mac platforms, I just haven't tested & didn't want to turn on without someone having done so. Btw, nice write up on JSC. :-) Created attachment 113342 [details]
Patch
Cool, I'm looking forward to playing around with this in more detail :) And thanks for the kind words, it has been fun to look into JSC, and I hope to hack on it more in the future. To move things along, the attached patch enables the DFG JIT on all x86-64 platforms. Perhaps Yuqiang can add x86-32 to the set when it is ready, too. For now, from my testing of GTK+ port on Linux x86 with DFG JIT turned on, I didn't observe any regressions comparing to ToT (including layout test, JavaScriptCore test, and JS benchmarks). Nice work, Yuqiang! And congrats on becoming a committer. The patch to be attached enables the DFG JIT on x86-64 platforms as well. Created attachment 114057 [details]
Patch
Sunspider: EST COMPARISON FROM TO DETAILS ============================================================================= ** TOTAL **: 1.074x as fast 162.0ms +/- 1.0% 150.9ms +/- 1.5% significant ============================================================================= 3d: - 25.4ms +/- 7.2% 25.1ms +/- 7.9% cube: ?? 10.4ms +/- 17.5% 10.5ms +/- 21.1% not conclusive: might be *1.010x as slow* morph: *1.057x as slow* 7.0ms +/- 0.0% 7.4ms +/- 5.0% significant raytrace: 1.111x as fast 8.0ms +/- 0.0% 7.2ms +/- 4.2% significant access: 1.72x as fast 20.6ms +/- 2.4% 12.0ms +/- 0.0% significant binary-trees: 2.00x as fast 2.0ms +/- 0.0% 1.0ms +/- 0.0% significant fannkuch: 1.70x as fast 10.2ms +/- 3.0% 6.0ms +/- 0.0% significant nbody: 1.80x as fast 5.4ms +/- 6.8% 3.0ms +/- 0.0% significant nsieve: 1.50x as fast 3.0ms +/- 0.0% 2.0ms +/- 0.0% significant bitops: 1.59x as fast 14.5ms +/- 2.6% 9.1ms +/- 2.5% significant 3bit-bits-in-byte: 2.00x as fast 2.0ms +/- 0.0% 1.0ms +/- 0.0% significant bits-in-byte: 2.65x as fast 5.3ms +/- 6.5% 2.0ms +/- 0.0% significant bitwise-and: 1.43x as fast 3.0ms +/- 0.0% 2.1ms +/- 10.8% significant nsieve-bits: - 4.2ms +/- 7.2% 4.0ms +/- 0.0% controlflow: ?? 1.0ms +/- 0.0% 1.2ms +/- 25.1% not conclusive: might be *1.20x as slow* recursive: ?? 1.0ms +/- 0.0% 1.2ms +/- 25.1% not conclusive: might be *1.20x as slow* crypto: *1.108x as slow* 10.2ms +/- 3.0% 11.3ms +/- 3.1% significant aes: *1.197x as slow* 6.1ms +/- 3.7% 7.3ms +/- 4.7% significant md5: - 2.1ms +/- 10.8% 2.0ms +/- 0.0% sha1: - 2.0ms +/- 0.0% 2.0ms +/- 0.0% date: - 18.0ms +/- 0.0% 17.9ms +/- 2.9% format-tofte: 1.042x as fast 10.0ms +/- 0.0% 9.6ms +/- 3.8% significant format-xparb: ?? 8.0ms +/- 0.0% 8.3ms +/- 5.8% not conclusive: might be *1.038x as slow* math: - 16.1ms +/- 1.4% 16.0ms +/- 0.0% cordic: *1.176x as slow* 5.1ms +/- 4.4% 6.0ms +/- 0.0% significant partial-sums: - 8.0ms +/- 0.0% 8.0ms +/- 0.0% spectral-norm: 1.50x as fast 3.0ms +/- 0.0% 2.0ms +/- 0.0% significant regexp: ?? 12.8ms +/- 2.4% 13.0ms +/- 0.0% not conclusive: might be *1.016x as slow* dna: ?? 12.8ms +/- 2.4% 13.0ms +/- 0.0% not conclusive: might be *1.016x as slow* string: *1.044x as slow* 43.4ms +/- 0.9% 45.3ms +/- 0.8% significant base64: 1.25x as fast 4.0ms +/- 0.0% 3.2ms +/- 9.4% significant fasta: - 6.0ms +/- 0.0% 6.0ms +/- 0.0% tagcloud: ?? 10.9ms +/- 2.1% 11.0ms +/- 0.0% not conclusive: might be *1.009x as slow* unpack-code: *1.149x as slow* 17.5ms +/- 2.2% 20.1ms +/- 1.1% significant validate-input: - 5.0ms +/- 0.0% 5.0ms +/- 0.0% V8: TEST COMPARISON FROM TO DETAILS ============================================================================= ** TOTAL **: 1.42x as fast 1070.4ms +/- 0.4% 751.8ms +/- 0.3% significant ============================================================================= v8: 1.42x as fast 1070.4ms +/- 0.4% 751.8ms +/- 0.3% significant crypto: 2.49x as fast 184.6ms +/- 1.0% 74.2ms +/- 2.2% significant deltablue: 1.49x as fast 256.7ms +/- 0.4% 172.7ms +/- 0.7% significant earley-boyer: 1.163x as fast 100.5ms +/- 0.4% 86.4ms +/- 0.6% significant raytrace: 1.093x as fast 65.6ms +/- 0.8% 60.0ms +/- 0.8% significant regexp: *1.011x as slow* 111.4ms +/- 0.3% 112.6ms +/- 0.6% significant richards: 1.82x as fast 217.9ms +/- 0.6% 119.7ms +/- 0.6% significant splay: 1.059x as fast 133.7ms +/- 0.7% 126.2ms +/- 0.9% significant Kraken: TEST COMPARISON FROM TO DETAILS ============================================================================= ** TOTAL **: 2.18x as fast 6721.2ms +/- 0.2% 3078.2ms +/- 0.3% significant ============================================================================= ai: 2.49x as fast 1126.0ms +/- 0.8% 451.5ms +/- 0.7% significant astar: 2.49x as fast 1126.0ms +/- 0.8% 451.5ms +/- 0.7% significant audio: 1.89x as fast 1694.9ms +/- 0.3% 897.2ms +/- 0.3% significant beat-detection: 2.54x as fast 482.1ms +/- 0.2% 190.0ms +/- 0.3% significant dft: 1.58x as fast 526.6ms +/- 0.2% 332.4ms +/- 0.5% significant fft: 2.86x as fast 359.6ms +/- 0.5% 125.6ms +/- 0.7% significant oscillator: 1.31x as fast 326.6ms +/- 0.9% 249.2ms +/- 0.5% significant imaging: 2.72x as fast 3009.1ms +/- 0.2% 1106.7ms +/- 0.5% significant gaussian-blur: 3.59x as fast 1914.2ms +/- 0.5% 533.2ms +/- 0.6% significant darkroom: 1.59x as fast 558.0ms +/- 0.9% 351.9ms +/- 0.6% significant desaturate: 2.42x as fast 536.9ms +/- 0.4% 221.6ms +/- 0.7% significant json: *1.058x as slow* 129.2ms +/- 2.2% 136.7ms +/- 1.0% significant parse-financial: - 56.2ms +/- 2.7% 55.1ms +/- 1.1% stringify-tinderbox: *1.118x as slow* 73.0ms +/- 2.1% 81.6ms +/- 1.3% significant stanford: 1.57x as fast 762.0ms +/- 0.4% 486.1ms +/- 0.5% significant crypto-aes: 1.39x as fast 143.7ms +/- 1.4% 103.1ms +/- 0.5% significant crypto-ccm: *1.018x as slow* 113.9ms +/- 1.2% 115.9ms +/- 1.2% significant crypto-pbkdf2: 1.92x as fast 380.4ms +/- 0.4% 197.8ms +/- 0.9% significant crypto-sha256-iterative: 1.79x as fast 124.0ms +/- 0.5% 69.3ms +/- 0.7% significant Comment on attachment 114057 [details] Patch Attachment 114057 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10379041 Created attachment 114067 [details]
Patch
The newly attached patch attempts to fix EFL. Comment on attachment 114067 [details]
Patch
Flipping cq+ as Andy is not committer yet.
Comment on attachment 114067 [details] Patch Clearing flags on attachment: 114067 Committed r99678: <http://trac.webkit.org/changeset/99678> All reviewed patches have been landed. Closing bug. Re-opening as the patch was rolled out due to build-errors of the following kind: The build errors were: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:734:30: error: variable 'tooBig' set but not used [-Werror=unused-but-set-variable] Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:708:12: error: variable 'valueGPR' set but not used [-Werror=unused-but-set-variable] Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:1552:27: error: variable 'functionCall' set but not used [-Werror=unused-but-set-variable] Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:2471:27: error: variable 'functionCall' set but not used [-Werror=unused-but-set-variable] We weren't 100% sure what the correct fixes were here :) Created attachment 114235 [details]
32 and 64 bit buildfix for stricter compilers
Comment on attachment 114235 [details] 32 and 64 bit buildfix for stricter compilers View in context: https://bugs.webkit.org/attachment.cgi?id=114235&action=review > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:734 > - MacroAssembler::Jump tooBig = m_jit.branch32(MacroAssembler::GreaterThan, scratchReg, TrustedImm32(0xff)); > + m_jit.branch32(MacroAssembler::GreaterThan, scratchReg, TrustedImm32(0xff)); This is the part I'm rather unsure about. It seems "odd" to leave an instruction in there that is unlinked. I added the patch at bug 71885 at the same time that Ossy wrote his patch. It removes the unlinked branch, FWIW. Comment on attachment 114235 [details] 32 and 64 bit buildfix for stricter compilers View in context: https://bugs.webkit.org/attachment.cgi?id=114235&action=review > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:-734 > - MacroAssembler::Jump tooBig = m_jit.branch32(MacroAssembler::GreaterThan, scratchReg, TrustedImm32(0xff)); I think this call is unnecessary. 1) The previous check <=, so the it is always > 2) The following code would be dead code. I think that it is working on x86 is a pure luck, since "jmp 0" jumps to the next instruction after the "jmp". Comment on attachment 114235 [details]
32 and 64 bit buildfix for stricter compilers
r=me
Necessary changes.
*** Bug 71885 has been marked as a duplicate of this bug. *** Just a note... The original patch probably needs to merge the latest change in DFG, in particular some new files are added with bug #71787 but the 64bit version may be missing in the Efl cmake list. Comment on attachment 114235 [details] 32 and 64 bit buildfix for stricter compilers Clearing flags on attachment: 114235 Committed r99693: <http://trac.webkit.org/changeset/99693> Created attachment 114257 [details]
Patch
Comment on attachment 114257 [details]
Patch
Updated, adding 64-bit OSR exit compiler to EFL build.
Comment on attachment 114257 [details]
Patch
r=me, I'll land it immediately.
Comment on attachment 114257 [details] Patch Clearing flags on attachment: 114257 Committed r99696: <http://trac.webkit.org/changeset/99696> All reviewed patches have been landed. Closing bug. (In reply to comment #18) > (From update of attachment 114235 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=114235&action=review > > > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:-734 > > - MacroAssembler::Jump tooBig = m_jit.branch32(MacroAssembler::GreaterThan, scratchReg, TrustedImm32(0xff)); > > I think this call is unnecessary. > 1) The previous check <=, so the it is always > > 2) The following code would be dead code. > I think that it is working on x86 is a pure luck, since "jmp 0" jumps to the next instruction after the "jmp". No. The first check is BelowOrEqual, which is an unsigned comparison. The second check is GreaterThan, which is a signed comparison. The tooBig branch should be linked to this line: m_jit.move(TrustedImm32(255), scratchReg); > No. The first check is BelowOrEqual, which is an unsigned comparison. The second check is GreaterThan, which is a signed comparison.
>
> The tooBig branch should be linked to this line:
>
> m_jit.move(TrustedImm32(255), scratchReg);
Oh, Intel naming convection: Below/Above unsigned, Greater/Less signed. Perhaps we should change these names to something less confusing.
(In reply to comment #29) > > No. The first check is BelowOrEqual, which is an unsigned comparison. The second check is GreaterThan, which is a signed comparison. > > > > The tooBig branch should be linked to this line: > > > > m_jit.move(TrustedImm32(255), scratchReg); > > Oh, Intel naming convection: Below/Above unsigned, Greater/Less signed. Perhaps we should change these names to something less confusing. Suggestions? |