[webkit-reviews] review denied: [Bug 130389] Remove ENABLE(LLINT) and ENABLE(LLINT_C_LOOP) guards : [Attachment 232247] Proposed patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has denied Dániel Bátyai
<dbatyai.u-szeged at partner.samsung.com>'s request for review:
Bug 130389: Remove ENABLE(LLINT) and ENABLE(LLINT_C_LOOP) guards
https://bugs.webkit.org/show_bug.cgi?id=130389

Attachment 232247: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=232247&action=review

------- Additional Comments from Mark Lam <mark.lam at apple.com>
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.