[webkit-reviews] review denied: [Bug 126393] CStack: Get the C Loop LLINT to build again. : [Attachment 220241] the patch.

[bugzilla-daemon at webkit.org]

Michael Saboff <msaboff at apple.com> has denied Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 126393: CStack: Get the C Loop LLINT to build again.
https://bugs.webkit.org/show_bug.cgi?id=126393

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

------- Additional Comments from Michael Saboff <msaboff at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=220241&action=review


r-

It seems to me that callToJavaScript should be calling the C Loop interpreter
and not the other way around.  By having CLoop::execute() call into
callToJavaScript (renamed for the CLoop in this patch to
_llint_call_to_javascript) the CLoop is diverging from the LLInt. 
CLoop::execute() is the entry point for the C Loop LLInt and that is what
callToJavaScript should call.

This patch also breaks 4 tests when compiled X86-64:
** The following JSC stress test failures have been introduced:
	jsc-layout-tests.yaml/js/script-tests/function-apply-aliased.js.layout
       
jsc-layout-tests.yaml/js/script-tests/function-apply-aliased.js.layout-no-llint

       
jsc-layout-tests.yaml/js/script-tests/function-apply-aliased.js.layout-no-cjit
       
jsc-layout-tests.yaml/js/script-tests/function-apply-aliased.js.layout-dfg-eage
r-no-cjit

> Source/JavaScriptCore/llint/LLIntThunks.cpp:97
> +EncodedJSValue callToJavaScript(void* executableAddress, VM* vm,
ProtoCallFrame* protoCallFrame)

This should be renamed to cLoopCallToJavaScript() or callToJavaScriptCLoop()
and don't rename callToJavaScript().

> Source/JavaScriptCore/llint/LLIntThunks.cpp:103
> +EncodedJSValue callToNativeFunction(void* executableAddress, VM* vm,
ProtoCallFrame* protoCallFrame)

Same comment as callToJavaScript above.  Rename this function not the one in
LowLevelInterpreter.asm

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:475
> +_llint_call_to_javascript:

Why a different name for the C Loop?

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:483
> +_llint_call_to_native_function:

Ditto.

> Source/JavaScriptCore/llint/LowLevelInterpreter.cpp:247
> +JSValue CLoop::execute(OpcodeID entryOpcodeID, void* executableAddress, VM*
vm, ProtoCallFrame* protoCallFrame, bool isInitializationPass)

I don't like that the C Loop interpreter uses the ProtoCallFrame.  We want it
to use a CallFrame just like the LLInt and other engines.

> Source/JavaScriptCore/offlineasm/cloop.rb:74
> +	       "t0"

Move the when "t0" to here as well.

> Source/JavaScriptCore/offlineasm/cloop.rb:75
>	   when "a1"

Move the when "t1" to here as well.