Bug 123615 – [Win] Enable DFG JIT.

• Component: Web Template Framework
• Status: RESOLVED FIXED

I have been running the last couple of days with DFG JIT enabled on Windows (release build), and haven't noticed any issues on normal, everyday browsing.
I suggest we go on and enable DFG JIT.

64-bit Windows won't get DFG JIT, as JIT is disabled there.

Created attachment 215723 [details]
Patch

[Build Bot]

Comment on attachment 215723 [details]
Patch

Attachment 215723 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/19888030

Win build fails to link, this should be fixed in 123614, which this bug depends on.

Created attachment 217051 [details]
Patch

Only enable DFG for 32-bit Windows.

[Build Bot]

Comment on attachment 217051 [details]
Patch

Attachment 217051 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/23658178

[Geoffrey Garen]

Comment on attachment 217051 [details]
Patch

Build failed.

[Brent Fulgham]

Build failure:

C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\lib32\JavaScriptCore.exp
     1>DFGCommonData.obj : error LNK2019: unresolved external symbol "public: void __thiscall JSC::DFG::JumpReplacement::fire(void)" (?fire@JumpReplacement@DFG@JSC@@QAEXXZ) referenced in function "public: bool __thiscall JSC::DFG::CommonData::invalidate(void)" (?invalidate@CommonData@DFG@JSC@@QAE_NXZ)
     1>DFGPlan.obj : error LNK2019: unresolved external symbol "bool __cdecl JSC::DFG::performWatchpointCollection(class JSC::DFG::Graph &)" (?performWatchpointCollection@DFG@JSC@@YA_NAAVGraph@12@@Z) referenced in function "private: enum JSC::DFG::Plan::CompilationPath __thiscall JSC::DFG::Plan::compileInThreadImpl(class JSC::DFG::LongLivedState &)" (?compileInThreadImpl@Plan@DFG@JSC@@AAE?AW4CompilationPath@123@AAVLongLivedState@23@@Z)
     1>DFGPlan.obj : error LNK2019: unresolved external symbol "bool __cdecl JSC::DFG::performInvalidationPointInjection(class JSC::DFG::Graph &)" (?performInvalidationPointInjection@DFG@JSC@@YA_NAAVGraph@12@@Z) referenced in function "private: enum JSC::DFG::Plan::CompilationPath __thiscall JSC::DFG::Plan::compileInThreadImpl(class JSC::DFG::LongLivedState &)" (?compileInThreadImpl@Plan@DFG@JSC@@AAE?AW4CompilationPath@123@AAVLongLivedState@23@@Z)
     1>C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\bin32\JavaScriptCore.dll : fatal error LNK1120: 3 unresolved externals
     1>Done Building Project "C:\cygwin\home\buildbot\WebKit\Source\JavaScriptCore\JavaScriptCore.vcxproj\JavaScriptCore.vcxproj" (build target(s)) -- FAILED.

(In reply to comment #8)
> Build failure:
> 
> C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\lib32\JavaScriptCore.exp
>      1>DFGCommonData.obj : error LNK2019: unresolved external symbol "public: void __thiscall JSC::DFG::JumpReplacement::fire(void)" (?fire@JumpReplacement@DFG@JSC@@QAEXXZ) referenced in function "public: bool __thiscall JSC::DFG::CommonData::invalidate(void)" (?invalidate@CommonData@DFG@JSC@@QAE_NXZ)
>      1>DFGPlan.obj : error LNK2019: unresolved external symbol "bool __cdecl JSC::DFG::performWatchpointCollection(class JSC::DFG::Graph &)" (?performWatchpointCollection@DFG@JSC@@YA_NAAVGraph@12@@Z) referenced in function "private: enum JSC::DFG::Plan::CompilationPath __thiscall JSC::DFG::Plan::compileInThreadImpl(class JSC::DFG::LongLivedState &)" (?compileInThreadImpl@Plan@DFG@JSC@@AAE?AW4CompilationPath@123@AAVLongLivedState@23@@Z)
>      1>DFGPlan.obj : error LNK2019: unresolved external symbol "bool __cdecl JSC::DFG::performInvalidationPointInjection(class JSC::DFG::Graph &)" (?performInvalidationPointInjection@DFG@JSC@@YA_NAAVGraph@12@@Z) referenced in function "private: enum JSC::DFG::Plan::CompilationPath __thiscall JSC::DFG::Plan::compileInThreadImpl(class JSC::DFG::LongLivedState &)" (?compileInThreadImpl@Plan@DFG@JSC@@AAE?AW4CompilationPath@123@AAVLongLivedState@23@@Z)
>      1>C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\bin32\JavaScriptCore.dll : fatal error LNK1120: 3 unresolved externals
>      1>Done Building Project "C:\cygwin\home\buildbot\WebKit\Source\JavaScriptCore\JavaScriptCore.vcxproj\JavaScriptCore.vcxproj" (build target(s)) -- FAILED.

These should be fixed by patch in bug 123614, which this bug depends on.

Created attachment 218204 [details]
Patch

Just reposted same patch to check that it builds cleanly now.

Output from run-javascriptcore-tests:

Results for Mozilla tests:
    0 regressions found.
    0 tests fixed.
    OK.

[Geoffrey Garen]

Given that the Windows JIT is often broken, and is still broken on 64bit, I'm skeptical about turning on the DFG on Windows.

(In reply to comment #13)
> Given that the Windows JIT is often broken, and is still broken on 64bit, I'm skeptical about turning on the DFG on Windows.

To be on the safe side, could we turn it on just for WinCairo?

[Geoffrey Garen]

Isn't WinCairo the platform that's currently experiencing 64bit problems? My point is that it's not so good to take a platform that's under-maintained and turn on new features.

[Brent Fulgham]

(In reply to comment #15)
> Isn't WinCairo the platform that's currently experiencing 64bit problems? My point is that it's not so good to take a platform that's under-maintained and turn on new features.

The Apple Windows port and the WinCairo port are nearly identical, especially at the JavaScriptCore level. They only differ by networking and graphics libraries.

We should be striving to get LINT, JIT, DFG, and other such features activated for 32- and 64-bit Windows.

[Brent Fulgham]

(In reply to comment #15)
> Isn't WinCairo the platform that's currently experiencing 64bit problems? My point is that it's not so good to take a platform that's under-maintained and turn on new features.

BTW -- those 64-bit errors were Windows (i.e., Apple Windows and WinCairo), and were only visible outside our time-zone.  The bugs peavo found have probably been present for a very long time.

At this point, I believe the 64-bit JIT is working, thought the jsc-stress-tests will give us a better idea.

[Geoffrey Garen]

> At this point, I believe the 64-bit JIT is working, thought the jsc-stress-tests will give us a better idea.

What is the status of run-jsc-stress-tests on Windows?

The current status of the Apple Windows buildbot is "failed jscore-test Exiting early after 500 failures. 33059 tests run. 500 failures 6 new passes".

> We should be striving to get LINT, JIT, DFG, and other such features activated for 32- and 64-bit Windows.

What should the priority of that work be relative to getting Windows to pass regression tests?

[Brent Fulgham]

(In reply to comment #18)
> > At this point, I believe the 64-bit JIT is working, thought the jsc-stress-tests will give us a better idea.
> 
> What is the status of run-jsc-stress-tests on Windows?

The script seems to run, though a number of tests are failing. I am investigating what's going on there. In some cases, it's lack of DFG support. I have filed https://bugs.webkit.org/show_bug.cgi?id=125111 

> The current status of the Apple Windows buildbot is "failed jscore-test Exiting early after 500 failures. 33059 tests run. 500 failures 6 new passes".

Looks like a machine problem; the bot has been rebooted.

We are seeing 6-8 failures on average, but the fact that "testapi" fails to run prevents us from seeing the results on the build bot. See https://bugs.webkit.org/show_bug.cgi?id=121972, which has not seen much progress in the last couple of months.

Filip suggested I get the jsc-stress-tests running, as they would give a better idea of things are not working and might shed light onto the VM destruction failure in that bug.
 
> > We should be striving to get LINT, JIT, DFG, and other such features activated for 32- and 64-bit Windows.
> 
> What should the priority of that work be relative to getting Windows to pass regression tests?

Point taken, we need to get regressions fixed first. Any hope of someone devoting some time to the testapi failure?

[Brent Fulgham]

This is another bug I'd like to get moving again. Is this patch still valid? It's pretty old at this point...

(In reply to comment #20)
> This is another bug I'd like to get moving again. Is this patch still valid? It's pretty old at this point...

Yes, I believe it's still valid. I will rebaseline it :)

Created attachment 231774 [details]
Patch

(In reply to comment #22)
> Created an attachment (id=231774) [details]
> Patch

Rebased.

Stress test results with DFG JIT enabled:

** The following JSC stress test failures have been introduced:
        mozilla-tests.yaml/ecma/GlobalObject/15.1.2.2-2.js.mozilla-dfg-eager-no-cjit-validate-phases
        mozilla-tests.yaml/ecma_3/Date/15.9.5.6.js.mozilla
        mozilla-tests.yaml/ecma_3/Date/15.9.5.6.js.mozilla-llint
        mozilla-tests.yaml/ecma_3/Date/15.9.5.6.js.mozilla-baseline
        mozilla-tests.yaml/ecma_3/Date/15.9.5.6.js.mozilla-dfg-eager-no-cjit-validate-phases
        stress/ftl-arithcos.js.default
        stress/ftl-arithcos.js.no-llint
        stress/ftl-arithcos.js.always-trigger-copy-phase
        stress/ftl-arithcos.js.no-cjit-validate-phases
        stress/ftl-arithcos.js.dfg-eager
        stress/ftl-arithcos.js.dfg-eager-no-cjit-validate
        jsc-layout-tests.yaml/js/script-tests/date-constructor.js.layout
        jsc-layout-tests.yaml/js/script-tests/date-constructor.js.layout-no-llint
        jsc-layout-tests.yaml/js/script-tests/date-constructor.js.layout-no-cjit
        jsc-layout-tests.yaml/js/script-tests/date-constructor.js.layout-dfg-eager-no-cjit
        jsc-layout-tests.yaml/js/script-tests/function-apply-many-args.js.layout
        jsc-layout-tests.yaml/js/script-tests/function-apply-many-args.js.layout-no-llint
        jsc-layout-tests.yaml/js/script-tests/function-apply-many-args.js.layout-no-cjit
        jsc-layout-tests.yaml/js/script-tests/function-apply-many-args.js.layout-dfg-eager-no-cjit
        jsc-layout-tests.yaml/js/script-tests/sort-stability.js.layout
        jsc-layout-tests.yaml/js/script-tests/sort-stability.js.layout-no-llint
        jsc-layout-tests.yaml/js/script-tests/sort-stability.js.layout-no-cjit
        jsc-layout-tests.yaml/js/script-tests/sort-stability.js.layout-dfg-eager-no-cjit

Results for JSC stress tests:
    23 failures found.

Stress test results with only baseline JIT enabled:

** The following JSC stress test failures have been introduced:
        mozilla-tests.yaml/ecma_3/Date/15.9.5.6.js.mozilla
        mozilla-tests.yaml/ecma_3/Date/15.9.5.6.js.mozilla-llint
        mozilla-tests.yaml/ecma_3/Date/15.9.5.6.js.mozilla-baseline
        mozilla-tests.yaml/ecma_3/Date/15.9.5.6.js.mozilla-dfg-eager-no-cjit-validate-phases
        stress/ftl-arithcos.js.default
        stress/ftl-arithcos.js.no-llint
        stress/ftl-arithcos.js.always-trigger-copy-phase
        stress/ftl-arithcos.js.no-cjit-validate-phases
        stress/ftl-arithcos.js.dfg-eager
        stress/ftl-arithcos.js.dfg-eager-no-cjit-validate
        jsc-layout-tests.yaml/js/script-tests/date-constructor.js.layout
        jsc-layout-tests.yaml/js/script-tests/date-constructor.js.layout-no-llint
        jsc-layout-tests.yaml/js/script-tests/date-constructor.js.layout-no-cjit
        jsc-layout-tests.yaml/js/script-tests/date-constructor.js.layout-dfg-eager-no-cjit
        jsc-layout-tests.yaml/js/script-tests/function-apply-many-args.js.layout
        jsc-layout-tests.yaml/js/script-tests/function-apply-many-args.js.layout-no-llint
        jsc-layout-tests.yaml/js/script-tests/function-apply-many-args.js.layout-no-cjit
        jsc-layout-tests.yaml/js/script-tests/function-apply-many-args.js.layout-dfg-eager-no-cjit
        jsc-layout-tests.yaml/js/script-tests/sort-stability.js.layout
        jsc-layout-tests.yaml/js/script-tests/sort-stability.js.layout-no-llint
        jsc-layout-tests.yaml/js/script-tests/sort-stability.js.layout-no-cjit
        jsc-layout-tests.yaml/js/script-tests/sort-stability.js.layout-dfg-eager-no-cjit

Results for JSC stress tests:
    22 failures found.

[Alex Christensen]

Do you know what is causing mozilla-tests.yaml/ecma/GlobalObject/15.1.2.2-2.js.mozilla-dfg-eager-no-cjit-validate-phases
Will this interfere with https://bugs.webkit.org/show_bug.cgi?id=123615

(In reply to comment #26)
> Do you know what is causing mozilla-tests.yaml/ecma/GlobalObject/15.1.2.2-2.js.mozilla-dfg-eager-no-cjit-validate-phases
> Will this interfere with https://bugs.webkit.org/show_bug.cgi?id=123615

No, I haven't looked into that one yet.

[Alex Christensen]

This is not specific to 32-bit, and it works with 64-bit after https://bugs.webkit.org/show_bug.cgi?id=130638

Created attachment 234363 [details]
Patch

(In reply to comment #29)
> Created an attachment (id=234363) [details]
> Patch

Included crash fixes for DFG on Win64, and fixes for stress test failures (also 64-bit).

With this patch, there are no regressions on both 32 and 64-bit Windows when enabling DFG:

DFG 32-bit:


** The following JSC stress test failures have been introduced:
        jsc-layout-tests.yaml/js/script-tests/date-constructor.js.layout
        jsc-layout-tests.yaml/js/script-tests/date-constructor.js.layout-dfg-eager-no-cjit
        jsc-layout-tests.yaml/js/script-tests/date-constructor.js.layout-no-cjit
        jsc-layout-tests.yaml/js/script-tests/date-constructor.js.layout-no-llint
        jsc-layout-tests.yaml/js/script-tests/sort-stability.js.layout
        jsc-layout-tests.yaml/js/script-tests/sort-stability.js.layout-dfg-eager-no-cjit
        jsc-layout-tests.yaml/js/script-tests/sort-stability.js.layout-no-cjit
        jsc-layout-tests.yaml/js/script-tests/sort-stability.js.layout-no-llint
        mozilla-tests.yaml/ecma_3/Date/15.9.5.6.js.mozilla
        mozilla-tests.yaml/ecma_3/Date/15.9.5.6.js.mozilla-baseline
        mozilla-tests.yaml/ecma_3/Date/15.9.5.6.js.mozilla-dfg-eager-no-cjit-validate-phases
        mozilla-tests.yaml/ecma_3/Date/15.9.5.6.js.mozilla-llint
        stress/ftl-arithcos.js.always-trigger-copy-phase
        stress/ftl-arithcos.js.default
        stress/ftl-arithcos.js.dfg-eager
        stress/ftl-arithcos.js.dfg-eager-no-cjit-validate
        stress/ftl-arithcos.js.no-cjit-validate-phases
        stress/ftl-arithcos.js.no-llint

Results for JSC stress tests:
    18 failures found.

------------------------------------------------------------------------------------------------

Baseline 32-bit:


** The following JSC stress test failures have been introduced:
        jsc-layout-tests.yaml/js/script-tests/date-constructor.js.layout
        jsc-layout-tests.yaml/js/script-tests/date-constructor.js.layout-dfg-eager-no-cjit
        jsc-layout-tests.yaml/js/script-tests/date-constructor.js.layout-no-cjit
        jsc-layout-tests.yaml/js/script-tests/date-constructor.js.layout-no-llint
        jsc-layout-tests.yaml/js/script-tests/sort-stability.js.layout
        jsc-layout-tests.yaml/js/script-tests/sort-stability.js.layout-dfg-eager-no-cjit
        jsc-layout-tests.yaml/js/script-tests/sort-stability.js.layout-no-cjit
        jsc-layout-tests.yaml/js/script-tests/sort-stability.js.layout-no-llint
        mozilla-tests.yaml/ecma_3/Date/15.9.5.6.js.mozilla
        mozilla-tests.yaml/ecma_3/Date/15.9.5.6.js.mozilla-baseline
        mozilla-tests.yaml/ecma_3/Date/15.9.5.6.js.mozilla-dfg-eager-no-cjit-validate-phases
        mozilla-tests.yaml/ecma_3/Date/15.9.5.6.js.mozilla-llint
        stress/ftl-arithcos.js.always-trigger-copy-phase
        stress/ftl-arithcos.js.default
        stress/ftl-arithcos.js.dfg-eager
        stress/ftl-arithcos.js.dfg-eager-no-cjit-validate
        stress/ftl-arithcos.js.no-cjit-validate-phases
        stress/ftl-arithcos.js.no-llint

Results for JSC stress tests:
    18 failures found.

------------------------------------------------------------------------------------------------

DFG 64-bit:

** The following JSC stress test failures have been introduced:
        jsc-layout-tests.yaml/js/script-tests/date-constructor.js.layout
        jsc-layout-tests.yaml/js/script-tests/date-constructor.js.layout-dfg-eager-no-cjit
        jsc-layout-tests.yaml/js/script-tests/date-constructor.js.layout-no-cjit
        jsc-layout-tests.yaml/js/script-tests/date-constructor.js.layout-no-llint
        jsc-layout-tests.yaml/js/script-tests/math.js.layout
        jsc-layout-tests.yaml/js/script-tests/math.js.layout-dfg-eager-no-cjit
        jsc-layout-tests.yaml/js/script-tests/math.js.layout-no-cjit
        jsc-layout-tests.yaml/js/script-tests/math.js.layout-no-llint
        jsc-layout-tests.yaml/js/script-tests/sort-stability.js.layout
        jsc-layout-tests.yaml/js/script-tests/sort-stability.js.layout-dfg-eager-no-cjit
        jsc-layout-tests.yaml/js/script-tests/sort-stability.js.layout-no-cjit
        jsc-layout-tests.yaml/js/script-tests/sort-stability.js.layout-no-llint
        mozilla-tests.yaml/ecma_3/Date/15.9.5.6.js.mozilla
        mozilla-tests.yaml/ecma_3/Date/15.9.5.6.js.mozilla-baseline
        mozilla-tests.yaml/ecma_3/Date/15.9.5.6.js.mozilla-dfg-eager-no-cjit-validate-phases
        mozilla-tests.yaml/ecma_3/Date/15.9.5.6.js.mozilla-llint
        stress/ftl-arithcos.js.always-trigger-copy-phase
        stress/ftl-arithcos.js.default
        stress/ftl-arithcos.js.dfg-eager
        stress/ftl-arithcos.js.dfg-eager-no-cjit-validate
        stress/ftl-arithcos.js.no-cjit-validate-phases
        stress/ftl-arithcos.js.no-llint

Results for JSC stress tests:
    22 failures found.

------------------------------------------------------------------------------------------------

Baseline 64-bit:

** The following JSC stress test failures have been introduced:
        jsc-layout-tests.yaml/js/script-tests/date-constructor.js.layout
        jsc-layout-tests.yaml/js/script-tests/date-constructor.js.layout-dfg-eager-no-cjit
        jsc-layout-tests.yaml/js/script-tests/date-constructor.js.layout-no-cjit
        jsc-layout-tests.yaml/js/script-tests/date-constructor.js.layout-no-llint
        jsc-layout-tests.yaml/js/script-tests/math.js.layout
        jsc-layout-tests.yaml/js/script-tests/math.js.layout-dfg-eager-no-cjit
        jsc-layout-tests.yaml/js/script-tests/math.js.layout-no-cjit
        jsc-layout-tests.yaml/js/script-tests/math.js.layout-no-llint
        jsc-layout-tests.yaml/js/script-tests/sort-stability.js.layout
        jsc-layout-tests.yaml/js/script-tests/sort-stability.js.layout-dfg-eager-no-cjit
        jsc-layout-tests.yaml/js/script-tests/sort-stability.js.layout-no-cjit
        jsc-layout-tests.yaml/js/script-tests/sort-stability.js.layout-no-llint
        mozilla-tests.yaml/ecma_3/Date/15.9.5.6.js.mozilla
        mozilla-tests.yaml/ecma_3/Date/15.9.5.6.js.mozilla-baseline
        mozilla-tests.yaml/ecma_3/Date/15.9.5.6.js.mozilla-dfg-eager-no-cjit-validate-phases
        mozilla-tests.yaml/ecma_3/Date/15.9.5.6.js.mozilla-llint
        stress/ftl-arithcos.js.always-trigger-copy-phase
        stress/ftl-arithcos.js.default
        stress/ftl-arithcos.js.dfg-eager
        stress/ftl-arithcos.js.dfg-eager-no-cjit-validate
        stress/ftl-arithcos.js.no-cjit-validate-phases
        stress/ftl-arithcos.js.no-llint

Results for JSC stress tests:
    22 failures found.

[Alex Christensen]

Comment on attachment 234363 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=234363&action=review

This is exciting!  Somebody from the JSC team will have to review it.

> Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:162
> +        // Note: this implementation supports up to 3 parameters.

Are 3 parameters always going to be enough?

> Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:164
> +        // Shift the parameters to the right.

Is there a way to make the parameters be in the right place without shifting them?

> Source/WTF/wtf/Platform.h:699
> +/* Enable the DFG JIT on X86 and X86_64.  Only tested on Mac, GNU/Linux, FreeBSD, and Windows. */

Couldn't we remove the Only tested... line entirely?

Created attachment 234498 [details]
Patch

(In reply to comment #32)
> (From update of attachment 234363 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234363&action=review
>

Thanks for looking into this :)
 
> This is exciting!  Somebody from the JSC team will have to review it.
> 
> > Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:162
> > +        // Note: this implementation supports up to 3 parameters.
> 
> Are 3 parameters always going to be enough?
> 

No, there is no guarantee for that.
Currently, there is only one caller (indirectly) of this method (JIT::callOperation(Sprt_JITOperation_EZ operation, int32_t op)),
which uses 2 parameters registers, so there is made room for one more parameter.
On the other hand, we probably don't want to add support for several more parameters than needed for performance reasons.

> > Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:164
> > +        // Shift the parameters to the right.
> 
> Is there a way to make the parameters be in the right place without shifting them?
> 

Good point, I believe that would require us to add a new method similar to

setupArgumentsWithExecState(TrustedImm32(op));

which skips using the first parameter register (which is needed for the stack pointer to the return value).

Maybe that is the best way to go?

> > Source/WTF/wtf/Platform.h:699
> > +/* Enable the DFG JIT on X86 and X86_64.  Only tested on Mac, GNU/Linux, FreeBSD, and Windows. */
> 
> Couldn't we remove the Only tested... line entirely?

Fixed in latest patch.

[Alex Christensen]

(In reply to comment #34)
> > Is there a way to make the parameters be in the right place without shifting them?
> > 
> 
> Good point, I believe that would require us to add a new method similar to
> 
> setupArgumentsWithExecState(TrustedImm32(op));
> 
> which skips using the first parameter register (which is needed for the stack pointer to the return value).
> 
> Maybe that is the best way to go?
Maybe.  Try it.  I'm not familiar enough with this code to say for sure, but I don't think shifting 3 parameter registers around is the best solution.

Created attachment 234647 [details]
Patch

(In reply to comment #35)
> (In reply to comment #34)
> > > Is there a way to make the parameters be in the right place without shifting them?
> > > 
> > 
> > Good point, I believe that would require us to add a new method similar to
> > 
> > setupArgumentsWithExecState(TrustedImm32(op));
> > 
> > which skips using the first parameter register (which is needed for the stack pointer to the return value).
> > 
> > Maybe that is the best way to go?
> Maybe.  Try it.  I'm not familiar enough with this code to say for sure, but I don't think shifting 3 parameter registers around is the best solution.

Added a method to setup the arguments which skips the first parameter register, in order to make room for the stack pointer to the return value.

[Alex Christensen]

Comment on attachment 234647 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=234647&action=review

This seems ok to me, but this needs to be reviewed by someone from the JavaScriptCore team.  I verified that it works as claimed.

> Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:163
> +        // Note: this implementation supports up to 3 parameters.

I think this comment needs to be removed.

[Mark Lam]

Comment on attachment 234647 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=234647&action=review

I'd suggested some renames of function names so that they are a little more consistent with each other, and reads more naturally.  Please also look into the storing of the CallerFrame pinter issue in callWithSlowPathReturnType.

> Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:155
> +    Call callSlowPathReturnType()

Let's rename "callSlowPathReturnType" to "callWithSlowPathReturnType".

>> Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:163
>> +        // Note: this implementation supports up to 3 parameters.
> 
> I think this comment needs to be removed.

I disagree.  I think this comment is needed.  I would prefer an assertion on the number of parameters instead, but I don't think there's a way to assert this right now given that the parameters are set up by a different function (i.e. setupArgumentsWithExecStateForCallWithSlowPathReturnType).

> Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:183
> +        // We also need to allocate the shadow space on the stack for the 4 parameter registers.
> +        // In addition, we need to allocate 16 bytes for the return value.
> +        sub64(TrustedImm32(6 * sizeof(int64_t)), X86Registers::esp);
> +
> +        // The first parameter register should contain a pointer to the stack allocated space for the return value.
> +        move(X86Registers::esp, X86Registers::ecx);
> +        add64(TrustedImm32(4 * sizeof(int64_t)), X86Registers::ecx);
> +
> +        DataLabelPtr label = moveWithPatch(TrustedImmPtr(0), scratchRegister);
> +        Call result = Call(m_assembler.call(scratchRegister), Call::Linkable);
> +
> +        add64(TrustedImm32(6 * sizeof(int64_t)), X86Registers::esp);
> +
> +        // Copy the return value into rax and rdx.
> +        load64(Address(X86Registers::eax, sizeof(int64_t)), X86Registers::edx);
> +        load64(Address(X86Registers::eax), X86Registers::eax);
> +
> +        ASSERT_UNUSED(label, differenceBetween(label, result) == REPTACH_OFFSET_CALL_R11);
> +        return result;

This code looks stale compared to call() below.  Specifically, I don't see you storing the ebp (CallerFrame*) on the stack, and adjusting the stack accordingly for that.  Is that not needed here?

> Source/JavaScriptCore/jit/CCallHelpers.h:802
> +        // On Windows, an argument maps to the same register (based on its position), even when there are floating point arguments.

I suggest rephrasing this comment slightly as "On Windows, arguments map to designated registers based on the argument positions, even when there are interlaced scalar and floating point arguments."

> Source/JavaScriptCore/jit/CCallHelpers.h:816
> +        // On Windows, an argument maps to the same register (based on its position), even when there are floating point arguments.

Ditto with comment.  I suggest using the rephrased comment above.

> Source/JavaScriptCore/jit/CCallHelpers.h:817
> +        // See http://msdn.microsoft.com/en-us/library/7572ztz4.aspx

This document at this url talks about "Return Values".  I think you meant to us this url instead; http://msdn.microsoft.com/en-us/library/zthk2dkh.aspx

> Source/JavaScriptCore/jit/CCallHelpers.h:1078
> +    ALWAYS_INLINE void setupArgumentsSlowPathReturnTypeCallWithExecState(TrustedImm32 arg1)

Please rename "setupArgumentsSlowPathReturnTypeCallWithExecState" to "setupArgumentsWithExecStateForCallWithSlowPathReturnType".

> Source/JavaScriptCore/jit/JIT.h:268
> +        Call appendSlowPathReturnTypeCall(const FunctionPtr& function)

Please rename "appendSlowPathReturnTypeCall" to "appendCallWithSlowPathReturnType".

> Source/JavaScriptCore/jit/JIT.h:270
> +            Call functionCall = callSlowPathReturnType();

Please rename "callSlowPathReturnType" to "callWithSlowPathReturnType".

> Source/JavaScriptCore/jit/JIT.h:661
> +        MacroAssembler::Call appendSlowPathReturnTypeCallWithExceptionCheck(const FunctionPtr&);

Please rename "appendSlowPathReturnTypeCallWithExceptionCheck" to "appendCallWithExceptionCheckAndSlowPathReturnType".

> Source/JavaScriptCore/jit/JITInlines.h:122
> +ALWAYS_INLINE MacroAssembler::Call JIT::appendSlowPathReturnTypeCallWithExceptionCheck(const FunctionPtr& function)

Please rename "appendSlowPathReturnTypeCallWithExceptionCheck" to "appendCallWithExceptionCheckAndSlowPathReturnType".

> Source/JavaScriptCore/jit/JITInlines.h:125
> +    MacroAssembler::Call call = appendSlowPathReturnTypeCall(function);

Please rename "appendSlowPathReturnTypeCall" to "appendCallWithSlowPathReturnType".

> Source/JavaScriptCore/jit/JITInlines.h:250
> +    setupArgumentsSlowPathReturnTypeCallWithExecState(TrustedImm32(op));
> +    return appendSlowPathReturnTypeCallWithExceptionCheck(operation);

Please rename "setupArgumentsSlowPathReturnTypeCallWithExecState" to "setupArgumentsWithExecStateForCallWithSlowPathReturnType", and "appendSlowPathReturnTypeCallWithExceptionCheck" to "appendCallWithExceptionCheckAndSlowPathReturnType".

Created attachment 234766 [details]
Patch

(In reply to comment #39)
> (From update of attachment 234647 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234647&action=review
> 

Thanks for reviewing :) The patch has been modified accordingly.
.
> 
> > Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:183
> > +        // We also need to allocate the shadow space on the stack for the 4 parameter registers.
> > +        // In addition, we need to allocate 16 bytes for the return value.
> > +        sub64(TrustedImm32(6 * sizeof(int64_t)), X86Registers::esp);
> > +
> > +        // The first parameter register should contain a pointer to the stack allocated space for the return value.
> > +        move(X86Registers::esp, X86Registers::ecx);
> > +        add64(TrustedImm32(4 * sizeof(int64_t)), X86Registers::ecx);
> > +
> > +        DataLabelPtr label = moveWithPatch(TrustedImmPtr(0), scratchRegister);
> > +        Call result = Call(m_assembler.call(scratchRegister), Call::Linkable);
> > +
> > +        add64(TrustedImm32(6 * sizeof(int64_t)), X86Registers::esp);
> > +
> > +        // Copy the return value into rax and rdx.
> > +        load64(Address(X86Registers::eax, sizeof(int64_t)), X86Registers::edx);
> > +        load64(Address(X86Registers::eax), X86Registers::eax);
> > +
> > +        ASSERT_UNUSED(label, differenceBetween(label, result) == REPTACH_OFFSET_CALL_R11);
> > +        return result;
> 
> This code looks stale compared to call() below.  Specifically, I don't see you storing the ebp (CallerFrame*) on the stack, and adjusting the stack accordingly for that.  Is that not needed here?
> 

I don't believe it's needed in this specific case, but I added it, since it will probably be required in the future.

[Mark Lam]

Comment on attachment 234766 [details]
Patch

r=me

(In reply to comment #42)
> (From update of attachment 234766 [details])
> r=me

Thanks!

[WebKit Commit Bot]

Comment on attachment 234766 [details]
Patch

Clearing flags on attachment: 234766

Committed r171005: <http://trac.webkit.org/changeset/171005>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.