[DFG][FTL] Support MapSet / SetAdd intrinsics
Created attachment 327304 [details] Patch
Created attachment 327305 [details] Patch
Created attachment 327306 [details] Patch
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
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 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
Created attachment 327308 [details] Patch
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
Committed r225072: <https://trac.webkit.org/changeset/225072>
<rdar://problem/35655251>
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.