Bug 130389 - Remove ENABLE(LLINT) and ENABLE(LLINT_C_LOOP) guards
Summary: Remove ENABLE(LLINT) and ENABLE(LLINT_C_LOOP) guards
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 130638
Blocks:
  Show dependency treegraph
 
Reported: 2014-03-18 03:36 PDT by Dániel Bátyai
Modified: 2014-06-19 09:33 PDT (History)
12 users (show)

See Also:


Attachments
Proposed patch (69.81 KB, patch)
2014-03-18 03:57 PDT, Dániel Bátyai
no flags Details | Formatted Diff | Diff
Proposed patch (73.27 KB, patch)
2014-05-29 07:15 PDT, Dániel Bátyai
mark.lam: review-
Details | Formatted Diff | Diff
Proposed patch (69.26 KB, patch)
2014-06-06 03:34 PDT, Dániel Bátyai
mark.lam: review-
Details | Formatted Diff | Diff
Proposed patch (70.76 KB, patch)
2014-06-19 05:35 PDT, Dániel Bátyai
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (523.48 KB, application/zip)
2014-06-19 07:15 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Dániel Bátyai 2014-03-18 03:36:47 PDT
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 "
Comment 1 Dániel Bátyai 2014-03-18 03:57:11 PDT
Created attachment 227025 [details]
Proposed patch
Comment 2 Dániel Bátyai 2014-05-29 07:15:05 PDT
Created attachment 232247 [details]
Proposed patch
Comment 3 Mark Lam 2014-05-29 07:55:58 PDT
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.
Comment 4 Dániel Bátyai 2014-06-06 03:34:03 PDT
Created attachment 232608 [details]
Proposed patch

Modified according to review.
Comment 5 Mark Lam 2014-06-17 11:51:03 PDT
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?
Comment 6 Dániel Bátyai 2014-06-19 05:35:11 PDT
Created attachment 233357 [details]
Proposed patch
Comment 7 Build Bot 2014-06-19 07:15:23 PDT
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
Comment 8 Build Bot 2014-06-19 07:15:29 PDT
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 9 Mark Lam 2014-06-19 09:01:49 PDT
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 10 WebKit Commit Bot 2014-06-19 09:33:00 PDT
Comment on attachment 233357 [details]
Proposed patch

Clearing flags on attachment: 233357

Committed r170147: <http://trac.webkit.org/changeset/170147>
Comment 11 WebKit Commit Bot 2014-06-19 09:33:06 PDT
All reviewed patches have been landed.  Closing bug.