Bug 151901

Summary:

FTL B3 should be able to make JS->JS calls

Product:

WebKit

Reporter:

Filip Pizlo <fpizlo>

Component:

JavaScriptCore

Assignee:

Filip Pizlo <fpizlo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

barraclough, benjamin, commit-queue, ggaren, mark.lam, mhahnenb, msaboff, nrotem, oliver, saam, sam

Priority:

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

151808

Attachments:

Description	Flags
work in progress	none
work in progress	none
it compiles!	none
more	none
moar	none
the patch	saam: review+

Filip Pizlo

Reported 2015-12-04 18:06:51 PST

Patch forthcoming.

Attachments
work in progress (21.92 KB, patch) 2015-12-04 18:07 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
work in progress (66.62 KB, patch) 2015-12-05 11:12 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
it compiles! (68.03 KB, patch) 2015-12-05 21:28 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
more (75.56 KB, patch) 2015-12-06 11:13 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
moar (84.62 KB, patch) 2015-12-06 12:53 PST, Filip Pizlo	no flags	Details Formatted Diff Diff
the patch (102.25 KB, patch) 2015-12-06 14:53 PST, Filip Pizlo	saam: review+	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Filip Pizlo

Comment 1 2015-12-04 18:07:30 PST

Created attachment 266694 [details] work in progress

Filip Pizlo

Comment 2 2015-12-05 11:12:14 PST

Created attachment 266720 [details] work in progress Getting close to done. I refactored a bunch of stuff that was making this unnecessarily weird to write.

Filip Pizlo

Comment 3 2015-12-05 21:28:58 PST

Created attachment 266725 [details] it compiles!

Filip Pizlo

Comment 4 2015-12-06 11:13:48 PST

Created attachment 266732 [details] more Previously it compiled because I didn't set FTL_USES_B3. :-/

Filip Pizlo

Comment 5 2015-12-06 12:53:44 PST

Created attachment 266734 [details] moar

Filip Pizlo

Comment 6 2015-12-06 14:53:12 PST

Created attachment 266737 [details] the patch

WebKit Commit Bot

Comment 7 2015-12-06 14:54:11 PST

Attachment 266737 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/testb3.cpp:3417: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:3444: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:3489: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:3524: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:3562: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:3622: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:3650: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:3674: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:3740: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:3744: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:3769: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:3795: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:3820: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:3848: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:3870: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:3902: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:3948: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:3988: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:3991: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4024: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4027: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4060: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4097: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4130: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4204: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4207: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4241: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4279: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4317: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4390: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4420: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4455: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4496: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4542: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4545: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4576: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:4679: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:4851: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:4872: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:4909: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:5423: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:5435: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:8049: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:8066: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/b3/B3StackmapGenerationParams.cpp:51: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/b3/B3StackmapGenerationParams.cpp:52: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/b3/B3StackmapGenerationParams.cpp:53: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/ftl/FTLOSRExit.cpp:87: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/ftl/FTLOSRExit.cpp:113: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/ftl/FTLOSRExit.cpp:114: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/b3/B3StackmapGenerationParams.h:73: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 51 in 29 files If any of these errors are false positives, please file a bug against check-webkit-style.

Saam Barati

Comment 8 2015-12-07 11:11:39 PST

Comment on attachment 266737 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=266737&action=review r=me > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:4855 > + // Make sure that the callee goes into GPR0. I would say why this is and not that you're doing this b/c the code below is duplicate of this comment. i.e, the virtual call trampoline expects callee in regT0 > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:4858 > + // Make this easy for ourselves: this adds a stack argument. I don't think this comment is needed. > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:4865 > + // Now put things into the stack. I don't think this comment is needed. > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:4886 > + // FIXME: If we were handling exceptions, then at this point we would ask our descriptor > + // to prepare and then we would modify the OSRExit data structure inside the > + // OSRExitHandle to link it up to this call. Do we have a bug number for this? If so, we should link to it here. > Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:5451 > + // Make sure that the effects are as we want. Again, I think this comment is a bit redundant.

Filip Pizlo

Comment 9 2015-12-07 11:18:51 PST

Landed in http://trac.webkit.org/changeset/193640

Note You need to log in before you can comment on or make changes to this bug.