Bug 130389 – Remove ENABLE(LLINT) and ENABLE(LLINT_C_LOOP) guards

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Dániel Bátyai]

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 "

[Dániel Bátyai]

Created attachment 227025 [details]
Proposed patch

[Dániel Bátyai]

Created attachment 232247 [details]
Proposed patch

[Mark Lam]

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.

[Dániel Bátyai]

Created attachment 232608 [details]
Proposed patch

Modified according to review.

[Mark Lam]

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?

[Dániel Bátyai]

Created attachment 233357 [details]
Proposed patch

[Build Bot]

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

[Build Bot]

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

[Mark Lam]

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.

[WebKit Commit Bot]

Comment on attachment 233357 [details]
Proposed patch

Clearing flags on attachment: 233357

Committed r170147: <http://trac.webkit.org/changeset/170147>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.