Bug 185682

Summary:

[JSC] JSC should have consistent InById IC

Product:

WebKit

Reporter:

Yusuke Suzuki <ysuzuki>

Component:

JavaScriptCore

Assignee:

Yusuke Suzuki <ysuzuki>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, dominik.infuehr, ews-watchlist, fpizlo, keith_miller, mark.lam, msaboff, ryanhaddad, saam, ticaiolima, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

185842

Bug Blocks:

185803

Attachments:

Description	Flags
WIP	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	fpizlo: review+

Yusuke Suzuki

Reported 2018-05-16 09:51:33 PDT

InById operation is now ad-hocly optimized. We should align it to GetById etc. This bug introduces op_in_by_id, and op_in_by_val. And DFG should have InById and InByVal. In DFG AI, and other places we are attempting to convert InByVal to InById. So current ad-hoc In IC can be refactored.

Attachments
WIP (105.15 KB, patch) 2018-05-18 16:40 PDT, Yusuke Suzuki	no flags	Details Formatted Diff Diff
Patch (83.42 KB, patch) 2018-05-19 07:42 PDT, Yusuke Suzuki	no flags	Details Formatted Diff Diff
Patch (83.45 KB, patch) 2018-05-19 08:28 PDT, Yusuke Suzuki	no flags	Details Formatted Diff Diff
Patch (92.31 KB, patch) 2018-05-19 09:38 PDT, Yusuke Suzuki	no flags	Details Formatted Diff Diff
Patch (92.35 KB, patch) 2018-05-19 10:11 PDT, Yusuke Suzuki	no flags	Details Formatted Diff Diff
Patch (91.40 KB, patch) 2018-05-19 11:54 PDT, Yusuke Suzuki	fpizlo: review+	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Yusuke Suzuki

Comment 1 2018-05-16 09:52:02 PDT

(In reply to Yusuke Suzuki from comment #0) > InById operation is now ad-hocly optimized. We should align it to GetById > etc. > This bug introduces op_in_by_id, and op_in_by_val. And DFG should have > InById and InByVal. > In DFG AI, and other places we are attempting to convert InByVal to InById. > So current ad-hoc In IC can be refactored. And we have a chance to fold it to constant :)

Yusuke Suzuki

Comment 2 2018-05-18 16:40:24 PDT

Created attachment 340758 [details] WIP

EWS Watchlist

Comment 3 2018-05-18 16:43:16 PDT

Attachment 340758 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:172: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:173: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JIT.cpp:275: Non-label code inside switch statements should be indented. [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:152: The parameter name "value" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 4 in 53 files If any of these errors are false positives, please file a bug against check-webkit-style.

Yusuke Suzuki

Comment 4 2018-05-19 07:42:40 PDT

Created attachment 340773 [details] Patch

EWS Watchlist

Comment 5 2018-05-19 07:45:41 PDT

Attachment 340773 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:172: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:173: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JIT.cpp:275: Non-label code inside switch statements should be indented. [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:152: The parameter name "value" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 4 in 48 files If any of these errors are false positives, please file a bug against check-webkit-style.

Yusuke Suzuki

Comment 6 2018-05-19 08:28:58 PDT

Created attachment 340775 [details] Patch

EWS Watchlist

Comment 7 2018-05-19 08:31:47 PDT

Attachment 340775 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:172: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:173: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JIT.cpp:275: Non-label code inside switch statements should be indented. [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:152: The parameter name "value" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 4 in 48 files If any of these errors are false positives, please file a bug against check-webkit-style.

Yusuke Suzuki

Comment 8 2018-05-19 09:38:33 PDT

Created attachment 340777 [details] Patch

EWS Watchlist

Comment 9 2018-05-19 09:42:14 PDT

Attachment 340777 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:172: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JIT.cpp:275: Non-label code inside switch statements should be indented. [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:152: The parameter name "value" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 3 in 56 files If any of these errors are false positives, please file a bug against check-webkit-style.

Yusuke Suzuki

Comment 10 2018-05-19 10:11:25 PDT

Created attachment 340779 [details] Patch

EWS Watchlist

Comment 11 2018-05-19 10:14:05 PDT

Attachment 340779 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:172: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JIT.cpp:275: Non-label code inside switch statements should be indented. [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:152: The parameter name "value" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 3 in 56 files If any of these errors are false positives, please file a bug against check-webkit-style.

Yusuke Suzuki

Comment 12 2018-05-19 11:54:02 PDT

Created attachment 340783 [details] Patch

EWS Watchlist

Comment 13 2018-05-19 11:57:45 PDT

Attachment 340783 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:172: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JIT.cpp:275: Non-label code inside switch statements should be indented. [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:152: The parameter name "value" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 3 in 56 files If any of these errors are false positives, please file a bug against check-webkit-style.

Filip Pizlo

Comment 14 2018-05-19 12:59:51 PDT

Comment on attachment 340783 [details] Patch I love this change. I was wanting to do this the whole time I was writing the instanceof IC.

Yusuke Suzuki

Comment 15 2018-05-19 13:21:36 PDT

Committed r231998: <https://trac.webkit.org/changeset/231998>

Radar WebKit Bug Importer

Comment 16 2018-05-19 13:22:21 PDT

<rdar://problem/40395093>

Yusuke Suzuki

Comment 17 2018-05-21 09:45:40 PDT

Committed r232017: <https://trac.webkit.org/changeset/232017>

Ryan Haddad

Comment 18 2018-05-21 14:29:46 PDT

(In reply to Yusuke Suzuki from comment #17) > Committed r232017: <https://trac.webkit.org/changeset/232017> 32-bit JSC tests are still failing assertions after this fix: https://build.webkit.org/builders/Apple%20High%20Sierra%2032-bit%20JSC%20%28BuildAndTest%29/builds/1957

WebKit Commit Bot

Comment 19 2018-05-21 14:49:10 PDT

Re-opened since this is blocked by bug 185842

Dominik Inführ

Comment 20 2018-05-21 15:12:52 PDT

I've fixed this patch to also work on 32-bit architectures in this patch: https://bugs.webkit.org/show_bug.cgi?id=185845.

Saam Barati

Comment 21 2018-05-21 16:52:43 PDT

Comment on attachment 340783 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340783&action=review > Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:181 > + generateFastCommon(jit, InlineAccess::sizeForPropertyAccess()); Don't we want a size tuned for "in"?

Yusuke Suzuki

Comment 22 2018-05-21 21:54:37 PDT

Committed r232047: <https://trac.webkit.org/changeset/232047>

Yusuke Suzuki

Comment 23 2018-05-21 21:56:30 PDT

Comment on attachment 340783 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340783&action=review >> Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:181 >> + generateFastCommon(jit, InlineAccess::sizeForPropertyAccess()); > > Don't we want a size tuned for "in"? I think this is enough for now, since the changes between load v.s. in is very trivial (load property v.s. move constant).

Caio Lima

Comment 24 2018-05-22 04:57:48 PDT

Sweet! I think it is a good time to get back work into https://bugs.webkit.org/show_bug.cgi?id=172888

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