Bug 185682

Summary: [JSC] JSC should have consistent InById IC
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: 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: P2 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+

Description Yusuke Suzuki 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.
Comment 1 Yusuke Suzuki 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 :)
Comment 2 Yusuke Suzuki 2018-05-18 16:40:24 PDT
Created attachment 340758 [details]
WIP
Comment 3 EWS Watchlist 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.
Comment 4 Yusuke Suzuki 2018-05-19 07:42:40 PDT
Created attachment 340773 [details]
Patch
Comment 5 EWS Watchlist 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.
Comment 6 Yusuke Suzuki 2018-05-19 08:28:58 PDT
Created attachment 340775 [details]
Patch
Comment 7 EWS Watchlist 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.
Comment 8 Yusuke Suzuki 2018-05-19 09:38:33 PDT
Created attachment 340777 [details]
Patch
Comment 9 EWS Watchlist 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.
Comment 10 Yusuke Suzuki 2018-05-19 10:11:25 PDT
Created attachment 340779 [details]
Patch
Comment 11 EWS Watchlist 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.
Comment 12 Yusuke Suzuki 2018-05-19 11:54:02 PDT
Created attachment 340783 [details]
Patch
Comment 13 EWS Watchlist 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.
Comment 14 Filip Pizlo 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.
Comment 15 Yusuke Suzuki 2018-05-19 13:21:36 PDT
Committed r231998: <https://trac.webkit.org/changeset/231998>
Comment 16 Radar WebKit Bug Importer 2018-05-19 13:22:21 PDT
<rdar://problem/40395093>
Comment 17 Yusuke Suzuki 2018-05-21 09:45:40 PDT
Committed r232017: <https://trac.webkit.org/changeset/232017>
Comment 18 Ryan Haddad 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
Comment 19 WebKit Commit Bot 2018-05-21 14:49:10 PDT
Re-opened since this is blocked by bug 185842
Comment 20 Dominik Inführ 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.
Comment 21 Saam Barati 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"?
Comment 22 Yusuke Suzuki 2018-05-21 21:54:37 PDT
Committed r232047: <https://trac.webkit.org/changeset/232047>
Comment 23 Yusuke Suzuki 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).
Comment 24 Caio Lima 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