Bug 123615

Summary:

[Win] Enable DFG JIT.

Product:

WebKit

Reporter:

peavo

Component:

Web Template Framework

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

achristensen, alex.christensen, benjamin, bfulgham, bunhere, cdumez, cmarcelo, commit-queue, ggaren, gyuyoung.kim, mark.lam, msaboff, sergio

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

Unspecified

Bug Depends on:

123614

Bug Blocks:

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

Description peavo 2013-11-01 01:33:39 PDT

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.

Comment 1 peavo 2013-11-01 01:38:56 PDT

Created attachment 215723 [details]
Patch

Comment 2 Build Bot 2013-11-01 02:23:45 PDT

Comment on attachment 215723 [details]
Patch

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

Comment 3 peavo 2013-11-01 02:36:58 PDT

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

Comment 4 peavo 2013-11-15 07:27:27 PST

Created attachment 217051 [details]
Patch

Comment 5 peavo 2013-11-15 07:28:11 PST

Only enable DFG for 32-bit Windows.

Comment 6 Build Bot 2013-11-15 08:13:31 PST

Comment on attachment 217051 [details]
Patch

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

Comment 7 Geoffrey Garen 2013-11-15 10:04:43 PST

Comment on attachment 217051 [details]
Patch

Build failed.

Comment 8 Brent Fulgham 2013-11-15 10:12:12 PST

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.

Comment 9 peavo 2013-11-15 12:10:41 PST

(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.

Comment 10 peavo 2013-12-02 11:54:09 PST

Created attachment 218204 [details]
Patch

Comment 11 peavo 2013-12-02 11:55:24 PST

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

Comment 12 peavo 2013-12-02 12:25:04 PST

Output from run-javascriptcore-tests:

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

Comment 13 Geoffrey Garen 2013-12-02 12:31:09 PST

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

Comment 14 peavo 2013-12-02 12:34:39 PST

(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?

Comment 15 Geoffrey Garen 2013-12-02 15:12:50 PST

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.

Comment 16 Brent Fulgham 2013-12-02 15:16:15 PST

(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.

Comment 17 Brent Fulgham 2013-12-02 15:19:33 PST

(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.

Comment 18 Geoffrey Garen 2013-12-02 15:30:40 PST

> 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?

Comment 19 Brent Fulgham 2013-12-02 16:11:04 PST

(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?

Comment 20 Brent Fulgham 2014-04-01 10:24:47 PDT

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

Comment 21 peavo 2014-04-01 11:31:50 PDT

(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 :)

Comment 22 peavo 2014-05-20 09:51:38 PDT

Created attachment 231774 [details]
Patch

Comment 23 peavo 2014-05-20 09:53:26 PDT

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

Rebased.

Comment 24 peavo 2014-05-20 09:54:06 PDT

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.

Comment 25 peavo 2014-05-20 09:55:06 PDT

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.

Comment 26 Alex Christensen 2014-05-20 10:13:41 PDT

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

Comment 27 peavo 2014-05-20 10:23:35 PDT

(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.

Comment 28 Alex Christensen 2014-06-11 23:08:50 PDT

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

Comment 29 peavo 2014-07-03 12:23:02 PDT

Created attachment 234363 [details]
Patch

Comment 30 peavo 2014-07-03 12:28:05 PDT

(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).

Comment 31 peavo 2014-07-03 12:30:52 PDT

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.

Comment 32 Alex Christensen 2014-07-07 09:50:29 PDT

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?

Comment 33 peavo 2014-07-07 11:06:03 PDT

Created attachment 234498 [details]
Patch

Comment 34 peavo 2014-07-07 11:09:41 PDT

(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.

Comment 35 Alex Christensen 2014-07-08 12:18:52 PDT

(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.

Comment 36 peavo 2014-07-09 11:22:03 PDT

Created attachment 234647 [details]
Patch

Comment 37 peavo 2014-07-09 11:24:45 PDT

(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.

Comment 38 Alex Christensen 2014-07-10 10:35:13 PDT

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.

Comment 39 Mark Lam 2014-07-10 14:23:24 PDT

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".

Comment 40 peavo 2014-07-11 08:55:53 PDT

Created attachment 234766 [details]
Patch

Comment 41 peavo 2014-07-11 09:00:39 PDT

(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.

Comment 42 Mark Lam 2014-07-11 11:16:08 PDT

Comment on attachment 234766 [details]
Patch

r=me

Comment 43 peavo 2014-07-11 11:20:19 PDT

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

Thanks!

Comment 44 WebKit Commit Bot 2014-07-11 11:53:40 PDT

Comment on attachment 234766 [details]
Patch

Clearing flags on attachment: 234766

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

Comment 45 WebKit Commit Bot 2014-07-11 11:53:48 PDT

All reviewed patches have been landed.  Closing bug.