In CPU(ARM_TRADITIONAL) backend, current implementation of ARMAssembler::prepareExecutableCopy allows to do direct branches if jump offset is short enough. This must be removed to allow these jumps to be relinked through ARMAssembler::relinkJump.
Created attachment 216171 [details] Remove direct branch from ARMAssembler::prepareExecutableCopy implementation.
Created attachment 216172 [details] Layout-jsc tests results without and with the patch (tested on r158741) Tested on ARM_TRADITIONAL board, r158741: - without the patch: 450 tests passed, 44 tests failed, 40 tests crashed. - with the patch: 489 tests passed, 5 tests failed, 0 tests crashed.
I am a bit worried about the performance. Did you try it with SunSpider and v8 benchmarks? I remember we have troubles with relink, so you need to mark those branches. That mark is tested by "if (!(iter->m_offset & 1))". Wouldn't be better to mark these branches as well?
No I didn't run these benchmarks because SunSpider and V8 are crashing with current implementation. I took subset of SunSpider and V8 to get tests that are not crashing with the current implementation: sunspider-1.0/controlflow-recursive.js sunspider-1.0/date-format-tofte.js sunspider-1.0/date-format-xparb.js sunspider-1.0/math-cordic.js sunspider-1.0/math-partial-sums.js sunspider-1.0/math-spectral-norm.js sunspider-1.0/regexp-dna.js sunspider-1.0/string-base64.js sunspider-1.0/string-fasta.js sunspider-1.0/string-tagcloud.js sunspider-1.0/string-unpack-code.js sunspider-1.0/string-validate-input.js v8-v6/v8-deltablue.js v8-v6/v8-earley-boyer.js v8-v6/v8-raytrace.js v8-v6/v8-regexp.js v8-v6/v8-richards.js v8-v6/v8-splay.js I run all of them 5 times, and here are the results: * r158741 without the patch: real 0m25.940s user 0m24.270s sys 0m1.540s * r158741 with the patch: real 0m27.035s user 0m25.330s sys 0m1.670s So you're right, there is a slight performance decrease. I'll take a look to check if it's possible to detect and mark those jumps as well.
Comment on attachment 216171 [details] Remove direct branch from ARMAssembler::prepareExecutableCopy implementation. I cq- the patch, I'll try to find a better solution.
Created attachment 216294 [details] Use specific implementation of PatchableJump for CPU(ARM_TRADITIONAL) This one should be much more better :)
(this 2nd patch also fixes the crashes, but does not impact the performance)
Comment on attachment 216294 [details] Use specific implementation of PatchableJump for CPU(ARM_TRADITIONAL) View in context: https://bugs.webkit.org/attachment.cgi?id=216294&action=review > Source/JavaScriptCore/jit/GPRInfo.h:-469 > - ASSERT(static_cast<unsigned>(reg) != InvalidGPRReg); Why don't we need this static_cast, InvalidGPRReg is an unsigned value that uses the full 32 bits. > Source/JavaScriptCore/jit/GPRInfo.h:-479 > - ASSERT(static_cast<unsigned>(reg) != InvalidGPRReg); Ditto
(In reply to comment #8) > Why don't we need this static_cast, InvalidGPRReg is an unsigned value that uses the full 32 bits. Because I get these warnings in debug build: In file included from /home/ubuntu/work/arm/webkit/Source/JavaScriptCore/bytecode/ValueRecovery.h:30:0, from /home/ubuntu/work/arm/webkit/Source/JavaScriptCore/bytecode/CodeOrigin.h:32, from /home/ubuntu/work/arm/webkit/Source/JavaScriptCore/dfg/DFGDesiredWatchpoints.h:33, from /home/ubuntu/work/arm/webkit/Source/JavaScriptCore/dfg/DFGPlan.h:37, from /home/ubuntu/work/arm/webkit/Source/JavaScriptCore/runtime/Executable.h:33, from /home/ubuntu/work/arm/webkit/Source/JavaScriptCore/runtime/JSFunctionInlines.h:29, from /home/ubuntu/work/arm/webkit/Source/JavaScriptCore/runtime/Operations.h:30, from /home/ubuntu/work/arm/webkit/Source/JavaScriptCore/API/JSBase.cpp:39: /home/ubuntu/work/arm/webkit/Source/JavaScriptCore/jit/GPRInfo.h: In static member function 'static unsigned int JSC::GPRInfo::toIndex(JSC::GPRReg)': /home/ubuntu/work/arm/webkit/Source/JavaScriptCore/jit/GPRInfo.h:469:9: attention : comparaison entre des expressions entières signée et non signée [-Wsign-compare] /home/ubuntu/work/arm/webkit/Source/JavaScriptCore/jit/GPRInfo.h: In static member function 'static const char* JSC::GPRInfo::debugName(JSC::GPRReg)': /home/ubuntu/work/arm/webkit/Source/JavaScriptCore/jit/GPRInfo.h:479:9: attention : comparaison entre des expressions entières signée et non signée [-Wsign-compare] where "attention : comparaison entre des expressions entières signée et non signée" means "warning comparison between signed and unsigned".
Comment on attachment 216294 [details] Use specific implementation of PatchableJump for CPU(ARM_TRADITIONAL) View in context: https://bugs.webkit.org/attachment.cgi?id=216294&action=review >> Source/JavaScriptCore/jit/GPRInfo.h:-469 >> - ASSERT(static_cast<unsigned>(reg) != InvalidGPRReg); > > Why don't we need this static_cast, InvalidGPRReg is an unsigned value that uses the full 32 bits. My bad. I was looking at the wrong declaration.
Comment on attachment 216294 [details] Use specific implementation of PatchableJump for CPU(ARM_TRADITIONAL) Rejecting attachment 216294 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 216294, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: uzz 3. patching file Source/JavaScriptCore/assembler/MacroAssembler.h patching file Source/JavaScriptCore/assembler/MacroAssemblerARM.h patching file Source/JavaScriptCore/jit/GPRInfo.h Hunk #1 FAILED at 466. Hunk #2 succeeded at 479 (offset 3 lines). 1 out of 2 hunks FAILED -- saving rejects to file Source/JavaScriptCore/jit/GPRInfo.h.rej Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Michael Saboff']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.appspot.com/results/21248422
Created attachment 216363 [details] Use specific implementation of PatchableJump for CPU(ARM_TRADITIONAL) (rebased on r158906)
Comment on attachment 216363 [details] Use specific implementation of PatchableJump for CPU(ARM_TRADITIONAL) (rebased on r158906) Attachment 216363 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/22638440
Comment on attachment 216363 [details] Use specific implementation of PatchableJump for CPU(ARM_TRADITIONAL) (rebased on r158906) cq=? as EFL-WK2 bot error is not related to this patch: Last 500 characters of output: 65%] Building CXX object Source/WebCore/CMakeFiles/WebCore.dir/html/HTMLFormElement.cpp.o c++: internal compiler error: Killed (program cc1plus) Please submit a full bug report, with preprocessed source if appropriate. See <file:///usr/share/doc/gcc-4.7/README.Bugs> for instructions. make[2]: *** [Source/WebCore/CMakeFiles/WebCore.dir/dom/Document.cpp.o] Error 4
Committed r158915: <http://trac.webkit.org/changeset/158915>