Mac already did that in r195562. At the moment B3 doesn't even build, but since we have enabled FTL with LLVM for the first this cycle, we should probably witch to B3 before the next stable release, or disable FTL for now again to avoid introducing a dependency that we will remove soon.
It should build fine, see bug153456 for details. But unfortunately there are zillion crashes, see bug153422 for details. Before this serious regression there were only 6 crashes. (bug152248)
Now there is only one JSC test failure with B3 based FTL JIT: bug153644 Once this bug is fixed, we can enable it by default and start removing LLVM dependency.
Created attachment 270193 [details] Patch
Since LLVM will now only be used by the iOS, I think we should get rid of FindLLVM.cmake and all the LLVM goo from the the JavaScriptCore CMake files as well.
Certainly we need the build to not fail when LLVM is not installed.
(In reply to comment #4) > Since LLVM will now only be used by the iOS, I think we should get rid of > FindLLVM.cmake and all the LLVM goo from the the JavaScriptCore CMake files > as well. I didn't realize that EFL supported FTL on AArch64. I don't know why EFL supports this but not GTK. Anyway, I guess we need to leave the LLVM CMake goo around for that case, unless you want to drop support for FTL or AArch64, but we should stop using it at least when it's not needed.
*** Bug 153720 has been marked as a duplicate of this bug. ***
Created attachment 270333 [details] Patch
This patch seems to work in both Epiphany and MiniBrowser, but I couldn't immediately figure out how to get JSC tests working, so....
Created attachment 270334 [details] Patch
Created attachment 270337 [details] Patch
(In reply to comment #6) > (In reply to comment #4) > > Since LLVM will now only be used by the iOS, I think we should get rid of > > FindLLVM.cmake and all the LLVM goo from the the JavaScriptCore CMake files > > as well. > > I didn't realize that EFL supported FTL on AArch64. I don't know why EFL > supports this but not GTK. IIRC, LLVM needed some patches for AArch64 that were not in LLVM 3.7. EFL enabled it adding those patches in jhbuild. But that doesn't work for us in production, so we were waiting for those patches to land in LLVM and require the new version for AArch64. > Anyway, I guess we need to leave the LLVM CMake goo around for that case, > unless you want to drop support for FTL or AArch64, but we should stop using > it at least when it's not needed. I think it's indeed simpler to just disable FTL on AArch64 for now, but maybe EFL guys have a good reason to keep using LLVM.
(In reply to comment #12) > (In reply to comment #6) > > (In reply to comment #4) > > > Since LLVM will now only be used by the iOS, I think we should get rid of > > > FindLLVM.cmake and all the LLVM goo from the the JavaScriptCore CMake files > > > as well. > > > > I didn't realize that EFL supported FTL on AArch64. I don't know why EFL > > supports this but not GTK. > > IIRC, LLVM needed some patches for AArch64 that were not in LLVM 3.7. EFL > enabled it adding those patches in jhbuild. But that doesn't work for us in > production, so we were waiting for those patches to land in LLVM and require > the new version for AArch64. FTL JIT worked more or less with the patched LLVM 3.5 on AArch64, but it wasn't enabled by default ever, because there were a few JSC stress test crashes. Bumping to LLVM 3.6 wasn't possible, because LLVM stucked in an infinite loop during compiling. (didn't investigate what was the problem) First and last, it isn't showstopper to remove the LLVM dependency and enable B3-FTL by default on X86_64 Linux platforms. And we can enable it on AArch64 once it is ready.
Comment on attachment 270337 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270337&action=review In general it looks good to me with some changes. > Source/JavaScriptCore/CMakeLists.txt:-1074 > - llvm/InitializeLLVM.cpp > - llvm/InitializeLLVMLinux.cpp > - llvm/InitializeLLVMPOSIX.cpp I think we will need these files for LLVMDisassembler, but I can fix it later. > Source/JavaScriptCore/ftl/FTLState.cpp:52 > +#if !FTL_USES_B3 > , context(llvm->ContextCreate()) > +#endif In this case context member would be uninitialized and unused. Let's add guard for it in the header file too. > Source/WTF/wtf/Platform.h:794 > We can remove these lines from Platform.h: #if PLATFORM(GTK) && HAVE(LLVM) && ENABLE(JIT) && !defined(ENABLE_FTL_JIT) && CPU(X86_64) #define ENABLE_FTL_JIT 1 #endif > Source/cmake/FindLLVM.cmake:-1 > -# Please don't remove Source/cmake/FindLLVM.cmake, because LLVMDisassembler still needs LLVM. > Source/cmake/OptionsEfl.cmake:280 > +if (ENABLE_FTL_JIT AND NOT WTF_CPU_X86_64) > + message(FATAL_ERROR "FTL JIT is only available on X86_64 architecture.") It would be great if we can enable FTL JIT explicitly with command line option on AArch64 too for testing purposes.
(In reply to comment #14) > I think we will need these files for LLVMDisassembler, but I can fix it > later. I didn't know about LLVMDisassembler. OK. > In this case context member would be uninitialized and unused. > Let's add guard for it in the header file too. OK. > We can remove these lines from Platform.h: > > #if PLATFORM(GTK) && HAVE(LLVM) && ENABLE(JIT) && !defined(ENABLE_FTL_JIT) > && CPU(X86_64) > #define ENABLE_FTL_JIT 1 > #endif Yeah, ENABLE_FTL_JIT is guaranteed to be defined for a long time now, that so condition will never be hit. > > Source/cmake/FindLLVM.cmake:-1 > > -# > > Please don't remove Source/cmake/FindLLVM.cmake, because LLVMDisassembler > still needs LLVM. OK. > It would be great if we can enable FTL JIT explicitly with command line > option on AArch64 too for testing purposes. OK, let's allow it when DEVELOPER_MODE is on.
(In reply to comment #15) > > It would be great if we can enable FTL JIT explicitly with command line > > option on AArch64 too for testing purposes. > > OK, let's allow it when DEVELOPER_MODE is on. I didn't mean enabling by default, because it isn't stable at all. Without FTL JIT we have 20-40 crashes long time ago on AArch64: bug151486, and ~120 crashes with enabled B3 FTL JIT. 94 WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_FTL_JIT PUBLIC ${WTF_CPU_X86_64}) This line enables FTL JIT on X86_64 by default, it's correct. But we didn't want to enable it on AArch64 yet, because JSC in itself it is unstable on AArch64 Linux a half year ago. We don't want to have more problem until somebody fixes bug151486. 279 if (ENABLE_FTL_JIT AND NOT WTF_CPU_X86_64) 280 message(FATAL_ERROR "FTL JIT is only available on X86_64 architecture.") I simply meant that we shouldn't trigger fatal error if a developer would like to enable FTL JIT on AArch64 with --ftl-jit command line option.
Right, I understood. I am planning to do this: if (ENABLE_FTL_JIT AND NOT WTF_CPU_X86_64 AND NOT DEVELOPER_MODE) message(FATAL_ERROR "FTL JIT is only available on X86_64 architecture.") endif () So if you pass -DENABLE_FTL_JIT and -DDEVELOPER_MODE (which should be passed by build-webkit) you can get FTL on AArch64, but otherwise you'll get stopped.
(In reply to comment #14) > In this case context member would be uninitialized and unused. > Let's add guard for it in the header file too. It gets used from various places... let's initialize it to nullptr and see if that works....
(In reply to comment #9) > This patch seems to work in both Epiphany and MiniBrowser, but I couldn't > immediately figure out how to get JSC tests working, so.... I ran 'run-javascriptcore-tests --debug --ftl' and got 30787/30787 passes. Some tests were disabled due to "missing Ruby features" but seems good enough to me.
Created attachment 270550 [details] Patch
Created attachment 270560 [details] Rebased patch Just made it apply on current trunk
Created attachment 270562 [details] Try again Same patch again now that build fix landed in r196057
I've just tried the patch but got a couple of failures: modules.yaml/modules/execution-order-self.js.ftl-eager-no-cjit-modules: Segmentation fault modules.yaml/modules/execution-order-self.js.ftl-eager-no-cjit-modules: ERROR: Unexpected exit code: 139 modules.yaml/modules/namespace.js.ftl-eager-modules: Segmentation fault modules.yaml/modules/namespace.js.ftl-eager-modules: ERROR: Unexpected exit code: 139 30901/30901 (failed 2) ** The following JSC stress test failures have been introduced: modules.yaml/modules/execution-order-self.js.ftl-eager-no-cjit-modules modules.yaml/modules/namespace.js.ftl-eager-modules Results for JSC stress tests: 2 failures found.
I've run Kraken, Octane, JetStream and Speedometer without problems, though, and getting significant better results with B3 in all cases expect Speedometer that I got the very same score with LLVM. I'm currently running my browser with B3 too, without problems so far.
Results on EFL port: ** The following JSC stress test failures have been introduced: modules.yaml/modules/execution-order-self.js.ftl-eager-no-cjit-modules Results for JSC stress tests: 1 failure found. Maybe it is a flakiness or a new regression.
We will need to add the following lines to Options<Efl|Gtk>.cmake : +if (USE_LLVM_DISASSEMBLER) + find_package(LLVM REQUIRED) + SET_AND_EXPOSE_TO_BUILD(HAVE_LLVM TRUE) +endif()
Comment on attachment 270562 [details] Try again View in context: https://bugs.webkit.org/attachment.cgi?id=270562&action=review > Source/JavaScriptCore/CMakeLists.txt:1076 > + list(APPEND JavaScriptCore_SOURCES > + disassembler/LLVMDisassembler.cpp > + disassembler/X86Disassembler.cpp We can remove these lines, these sources are added to JavaScriptCore_SOURCES unconditionally. > Source/JavaScriptCore/dfg/DFGPlan.cpp:458 > +#if !FTL_USES_B3 || USE(LLVM_DISASSEMBLER) How is this file related to LLVM_DISASSEMBLER? > Source/JavaScriptCore/dfg/DFGPlan.cpp:464 > +#if !FTL_USES_B3 || USE(LLVM_DISASSEMBLER) ditto > Source/JavaScriptCore/dfg/DFGPlan.cpp:471 > +#if !FTL_USES_B3 || USE(LLVM_DISASSEMBLER) ditto > Source/JavaScriptCore/ftl/FTLState.cpp:41 > +#if !FTL_USES_B3 > +#include <llvm/InitializeLLVM.h> > +#endif llvm/InitializeLLVM.h is guarded properly, we don't need conditional include.
We also need to change if (ENABLE_FTL_JIT) with if (USE_LLVM_DISASSEMBLER) in PlatformGTK.cmake
(In reply to comment #25) > Results on EFL port: > > ** The following JSC stress test failures have been introduced: > modules.yaml/modules/execution-order-self.js.ftl-eager-no-cjit-modules > > Results for JSC stress tests: > 1 failure found. > > > Maybe it is a flakiness or a new regression. Yes, maybe, I've just run it again and I got this one only as well. I'm using it in my browser without problems, so I think we can land the patch and fix tests failing afterwards.
(In reply to comment #25) > Results on EFL port: > > ** The following JSC stress test failures have been introduced: > modules.yaml/modules/execution-order-self.js.ftl-eager-no-cjit-modules > > Results for JSC stress tests: > 1 failure found. > > > Maybe it is a flakiness or a new regression. Is it caused before enabling B3?
(In reply to comment #30) > (In reply to comment #25) > > Results on EFL port: > > > > ** The following JSC stress test failures have been introduced: > > modules.yaml/modules/execution-order-self.js.ftl-eager-no-cjit-modules > > > > Results for JSC stress tests: > > 1 failure found. > > > > > > Maybe it is a flakiness or a new regression. > > Is it caused before enabling B3? Linux platforms still use LLVM-FTL without any failures, but we got this crash with the proposed patch which tries to enable B3. But previously (near a week before) I haven't seen any failures with enabled B3, that's why I think it can be a regression.
(In reply to comment #31) > (In reply to comment #30) > > (In reply to comment #25) > > > Results on EFL port: > > > > > > ** The following JSC stress test failures have been introduced: > > > modules.yaml/modules/execution-order-self.js.ftl-eager-no-cjit-modules > > > > > > Results for JSC stress tests: > > > 1 failure found. > > > > > > > > > Maybe it is a flakiness or a new regression. > > > > Is it caused before enabling B3? > > Linux platforms still use LLVM-FTL without any failures, but > we got this crash with the proposed patch which tries to enable B3. > > But previously (near a week before) I haven't seen any failures > with enabled B3, that's why I think it can be a regression. And that's why I think we should land this in trunk asap, while the patch is hanging here new regressions in linux might be introduced without nobody noticing it :-)
(In reply to comment #32) > And that's why I think we should land this in trunk asap, while the patch is > hanging here new regressions in linux might be introduced without nobody > noticing it :-) Yeah, landing this patch is very nice to me too ;) I've just applied this patch, and built JSC and execute it infinitely by "test.rb" loop { system "WebKitBuild/Release/bin/jsc -m Source/JavaScriptCore/tests/modules/execution-order-self.js" } But I didn't see any segv yet. Once it is caused, is it possible to paste its stack trace? (A separated issue is nice).
Created attachment 270579 [details] Addressed review comments
Attachment 270579 [details] did not pass style-queue: ERROR: Source/cmake/OptionsEfl.cmake:282: One space between command "endif" and its parentheses, should be "endif (" [whitespace/parentheses] [5] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 270580 [details] Fix "coding" style in EFL cmake file
Comment on attachment 270580 [details] Fix "coding" style in EFL cmake file LGTM, let's do the B3!
Comment on attachment 270580 [details] Fix "coding" style in EFL cmake file Clearing flags on attachment: 270580 Committed r196077: <http://trac.webkit.org/changeset/196077>
All reviewed patches have been landed. Closing bug.
Comment on attachment 270580 [details] Fix "coding" style in EFL cmake file View in context: https://bugs.webkit.org/attachment.cgi?id=270580&action=review > Source/cmake/OptionsEfl.cmake:94 > -WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_FTL_JIT PUBLIC ${ENABLE_FTL_DEFAULT}) > +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_FTL_JIT PUBLIC ${WTF_CPU_X86_64}) It didn't work on non X86_64, because WTF_CPU_X86_64 is undefined, not OFF. Reverted this part of the patch in https://trac.webkit.org/changeset/196113