[webkit-reviews] review granted: [Bug 179858] [DFG][FTL] Support MapSet / SetAdd intrinsics : [Attachment 327308] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 179858: [DFG][FTL] Support MapSet / SetAdd intrinsics
https://bugs.webkit.org/show_bug.cgi?id=179858

Attachment 327308: Patch

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




--- Comment #8 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 327308
  --> https://bugs.webkit.org/attachment.cgi?id=327308
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