185682 – [JSC] JSC should have consistent InById IC

RESOLVED FIXED 185682

[JSC] JSC should have consistent InById IC

https://bugs.webkit.org/show_bug.cgi?id=185682

Summary [JSC] JSC should have consistent InById IC

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.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware Unspecified

OS Unspecified

Product WebKit

Component JavaScriptCore

Assignee

Yusuke Suzuki

Reported

2018-05-16 09:51 PDT

Modified

2018-05-22 04:57 PDT History

CC List

11 users Show

URL

Keywords InRadar

Depends on

185842

Blocks

185803

Dependencies

tree graph