In https://bugs.webkit.org/show_bug.cgi?id=129429#c28 " FYI, we already always require ENABLE(LLINT). It's just a matter of ENABLE(JIT) or ENABLE(LLINT_C_LOOP) which is mutually exclusive. Maybe the time has come for us to remove all references to ENABLE(LLINT) and ENABLE(LLINT_C_LOOP) (inferred by !ENABLE(JIT)) everywhere "
Created attachment 227025 [details] Proposed patch
Created attachment 232247 [details] Proposed patch
Comment on attachment 232247 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=232247&action=review I've partially reviewed this patch from the bottom up. There's a fundamental error in the patch, that is, it assumes that we'll always run with the LLINT enabled. This is not true. We always build with the LLINT because it provides some of the bootstrapping glue code implementation for the JIT. However, we do not always run with the LLINT enabled. It is still possible (and desirable ... especially for testing purposes) to be able to run with the LLINT disabled. Hence, we should not be removing Options::useLLInt(). > Source/JavaScriptCore/runtime/Executable.cpp:307 > - if (Options::useLLInt()) > - setupLLInt(vm, codeBlock.get()); > - else > - setupJIT(vm, codeBlock.get()); > - > + setupLLInt(vm, codeBlock.get()); This change is wrong. Please revert. > Source/JavaScriptCore/runtime/Options.cpp:-214 > - Options::useLLInt() = true; This change is wrong. Please revert. > Source/JavaScriptCore/runtime/Options.cpp:-221 > -#if !ENABLE(LLINT) > - Options::useLLInt() = false; > -#endif This change is wrong. Please revert. > Source/JavaScriptCore/runtime/Options.h:-95 > - v(bool, useLLInt, true) \ This change is wrong. Please revert. > Source/JavaScriptCore/runtime/VM.h:318 > -#if ENABLE(JIT) && ENABLE(LLINT) > - bool canUseJIT() { return m_canUseJIT; } > -#elif ENABLE(JIT) > +#if ENABLE(JIT) > bool canUseJIT() { return true; } // jit only This change is wrong. Should return m_canUseJIT. > Source/JavaScriptCore/tests/stress/deleteAllCompiledCode.js:-2 > -//@ runNoLLInt > - This change is wrong. Please revert. > Source/WTF/wtf/Platform.h:728 > +/* If the baseline jit is not available, then disable upper tiers aswell: */ s/aswell/as well/. > Tools/Scripts/run-jsc-stress-tests:-590 > -def runNoLLInt > - run("no-llint", "--useLLInt=false") > -end > - This change is wrong. Please revert. > Tools/Scripts/run-jsc-stress-tests:-661 > - runNoLLInt This change is wrong. Please revert. > Tools/Scripts/run-jsc-stress-tests:-696 > - runNoLLInt This change is wrong. Please revert. > Tools/Scripts/run-jsc-stress-tests:-740 > -def runLayoutTestNoLLInt > - runLayoutTest("no-llint", "--useLLInt=false") > -end > - This change is wrong. Please revert. > Tools/Scripts/run-jsc-stress-tests:-763 > - runLayoutTestNoLLInt This change is wrong. Please revert. > Tools/Scripts/run-jsc-stress-tests:826 > - runMozillaTest("baseline", mode, extraFiles, "--useLLInt=false", "--useDFGJIT=false") > + runMozillaTest("baseline", mode, extraFiles, "--useDFGJIT=false") This is wrong. The --useLLInt=false is still valid. Just because we always build with it doesn't mean we have to run with it.
Created attachment 232608 [details] Proposed patch Modified according to review.
Comment on attachment 232608 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=232608&action=review Almost there, but need a few more changes. Please rebase to ToT while you're at it. Thanks. > Source/JavaScriptCore/CMakeLists.txt:628 > ) > target_link_libraries(LLIntOffsetsExtractor WTF) You unindented everything else in this previous "if (ENABLE_LLINT)" section, but left the above block indented. Is there a reason for this? > Source/JavaScriptCore/interpreter/JSStack.cpp:47 > +#endif // ENABLE(JIT) I think this should match the #if above i.e. "#endif // !ENABLE(JIT)". Same with all the other cases above and below if a comment is needed after the #endif. > Source/JavaScriptCore/interpreter/JSStack.cpp:66 > +#endif // ENABLE(JIT) Ditto. > Source/JavaScriptCore/interpreter/JSStack.cpp:161 > +#endif // ENABLE(JIT) Ditto. > Source/JavaScriptCore/interpreter/JSStack.h:123 > +#endif // ENABLE(JIT) Ditto. > Source/JavaScriptCore/llint/LLIntOpcode.h:34 > +#else // ENABLE(JIT) !cloop After reading this, I think it's better to remove all the "cloop" and "!cloop" comments in the patch. After all, ENABLE(JIT) implies !cloop and vice versa. Hence, there's no info gained by having all the "cloop" comments. Can you remove them please?
Created attachment 233357 [details] Proposed patch
Comment on attachment 233357 [details] Proposed patch Attachment 233357 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5339658883956736 New failing tests: media/W3C/video/networkState/networkState_during_loadstart.html
Created attachment 233359 [details] Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-09 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 233357 [details] Proposed patch r=me. I don’t think the mac-wk2-ews failure (media/W3C/video/networkState/networkState_during_loadstart.html) is related to this patch since this patch doesn’t change any code behavior.
Comment on attachment 233357 [details] Proposed patch Clearing flags on attachment: 233357 Committed r170147: <http://trac.webkit.org/changeset/170147>
All reviewed patches have been landed. Closing bug.