Bug 179858 – [DFG][FTL] Support MapSet / SetAdd intrinsics

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[DFG][FTL] Support MapSet / SetAdd intrinsics

[Yusuke Suzuki]

Created attachment 327304 [details]
Patch

[Yusuke Suzuki]

Created attachment 327305 [details]
Patch

[Yusuke Suzuki]

Created attachment 327306 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Yusuke Suzuki]

Created attachment 327308 [details]
Patch

[Saam Barati]

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

[Yusuke Suzuki]

Committed r225072: <https://trac.webkit.org/changeset/225072>

[Radar WebKit Bug Importer]

<rdar://problem/35655251>

[Yusuke Suzuki]

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.