Bug 179858 - [DFG][FTL] Support MapSet / SetAdd intrinsics
Summary: [DFG][FTL] Support MapSet / SetAdd intrinsics
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-11-18 01:01 PST by Yusuke Suzuki
Modified: 2017-11-21 02:41 PST (History)
11 users (show)

See Also:


Attachments
Patch (28.41 KB, patch)
2017-11-18 01:14 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (28.41 KB, patch)
2017-11-18 01:23 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (29.46 KB, patch)
2017-11-18 01:34 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews116 for mac-elcapitan (3.12 MB, application/zip)
2017-11-18 03:04 PST, EWS Watchlist
no flags Details
Patch (32.51 KB, patch)
2017-11-18 03:53 PST, Yusuke Suzuki
saam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2017-11-18 01:01:25 PST
[DFG][FTL] Support MapSet / SetAdd intrinsics
Comment 1 Yusuke Suzuki 2017-11-18 01:14:25 PST
Created attachment 327304 [details]
Patch
Comment 2 Yusuke Suzuki 2017-11-18 01:23:04 PST
Created attachment 327305 [details]
Patch
Comment 3 Yusuke Suzuki 2017-11-18 01:34:28 PST
Created attachment 327306 [details]
Patch
Comment 4 EWS Watchlist 2017-11-18 03:04:40 PST
Comment on attachment 327306 [details]
Patch

Attachment 327306 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5289170

New failing tests:
inspector/canvas/recording-2d.html
Comment 5 EWS Watchlist 2017-11-18 03:04:41 PST
Created attachment 327307 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 6 EWS Watchlist 2017-11-18 03:08:16 PST
Comment on attachment 327306 [details]
Patch

Attachment 327306 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/5289190

New failing tests:
stress/modify-set-during-iteration.js.ftl-eager-no-cjit
stress/SharedArrayBuffer-opt.js.ftl-no-cjit-validate-sampling-profiler
stress/SharedArrayBuffer-opt.js.ftl-eager-no-cjit
stress/modify-map-during-iteration.js.ftl-no-cjit-validate-sampling-profiler
stress/modify-map-during-iteration.js.ftl-eager-no-cjit
microbenchmarks/map-has-get-cse-opportunity.js.ftl-eager-no-cjit-b3o1
stress/modify-map-during-iteration.js.dfg-eager-no-cjit-validate
microbenchmarks/map-has-get-cse-opportunity.js.dfg-eager-no-cjit-validate
stress/map-iteration.js.ftl-eager-no-cjit-b3o1
stress/SharedArrayBuffer-opt.js.no-cjit-validate-phases
slowMicrobenchmarks.yaml/slowMicrobenchmarks/large-map-iteration-with-additions.js.ftl-no-cjit-validate-sampling-profiler
stress/SharedArrayBuffer-opt.js.dfg-eager-no-cjit-validate
microbenchmarks/map-has-get-cse-opportunity.js.ftl-eager-no-cjit
stress/SharedArrayBuffer-opt.js.dfg-maximal-flush-validate-no-cjit
stress/modify-set-during-iteration.js.dfg-eager-no-cjit-validate
airjs-tests.yaml/stress-test.js.ftl-eager-no-cjit
slowMicrobenchmarks.yaml/slowMicrobenchmarks/large-map-iteration-with-additions.js.no-cjit
stress/map-iteration.js.dfg-eager-no-cjit-validate
stress/map-iteration.js.ftl-eager-no-cjit
stress/modify-map-during-iteration.js.ftl-eager-no-cjit-b3o1
stress/SharedArrayBuffer-opt.js.ftl-no-cjit-no-inline-validate
stress/modify-set-during-iteration.js.ftl-eager-no-cjit-b3o1
stress/modify-map-during-iteration.js.no-cjit-validate-phases
stress/modify-map-during-iteration.js.ftl-no-cjit-no-inline-validate
stress/SharedArrayBuffer-opt.js.ftl-eager-no-cjit-b3o1
airjs-tests.yaml/stress-test.js.ftl-no-cjit
stress/SharedArrayBuffer-opt.js.ftl-no-cjit-no-put-stack-validate
stress/modify-map-during-iteration.js.ftl-no-cjit-no-put-stack-validate
Comment 7 Yusuke Suzuki 2017-11-18 03:53:27 PST
Created attachment 327308 [details]
Patch
Comment 8 Saam Barati 2017-11-20 09:19:39 PST
Comment on attachment 327308 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=327308&action=review

r=me with a question and a request:
Please add a test where you verify we don’t CSE the map load, something like:
set.has(key)
set.add(key)
set.has(key)

> Source/JavaScriptCore/dfg/DFGClobberize.h:1648
> +        write(MiscFields);

It would be good at some point to def this field so it can participate in CSE. I understand it’s a bit tricky now because of how we define get in terms of IR

> Source/JavaScriptCore/dfg/DFGOperations.cpp:2563
> +    jsCast<JSSet*>(set)->addNormalized(exec, normalizeMapKey(JSValue::decode(key)), JSValue(), hash);

Does MapHash implicitly do normalizeMapKey? If so, it’s probably worth adding that to the IR explicitly. That way we remove normalizing repeatedly the same key

> Source/JavaScriptCore/runtime/HashMapImpl.h:444
> +    ALWAYS_INLINE void addNormalized(ExecState* exec, JSValue key, JSValue value, int32_t hash)

Let’s add a debug assert that the hash is what we expect
Comment 9 Yusuke Suzuki 2017-11-21 02:40:43 PST
Committed r225072: <https://trac.webkit.org/changeset/225072>
Comment 10 Radar WebKit Bug Importer 2017-11-21 02:41:30 PST
<rdar://problem/35655251>
Comment 11 Yusuke Suzuki 2017-11-21 02:41:52 PST
Comment on attachment 327308 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=327308&action=review

Great, I've added tests for CSE.

>> Source/JavaScriptCore/dfg/DFGClobberize.h:1648
>> +        write(MiscFields);
> 
> It would be good at some point to def this field so it can participate in CSE. I understand it’s a bit tricky now because of how we define get in terms of IR

Oh, that's good. I've opened a bug for this.

>> Source/JavaScriptCore/dfg/DFGOperations.cpp:2563
>> +    jsCast<JSSet*>(set)->addNormalized(exec, normalizeMapKey(JSValue::decode(key)), JSValue(), hash);
> 
> Does MapHash implicitly do normalizeMapKey? If so, it’s probably worth adding that to the IR explicitly. That way we remove normalizing repeatedly the same key

That sounds nice. I think we should do this in a separate patch since it requires a new DFG node. I've opened a bug for this.
https://bugs.webkit.org/show_bug.cgi?id=179912

>> Source/JavaScriptCore/runtime/HashMapImpl.h:444
>> +    ALWAYS_INLINE void addNormalized(ExecState* exec, JSValue key, JSValue value, int32_t hash)
> 
> Let’s add a debug assert that the hash is what we expect

Nice, added.