Bug 179912 - [DFG] Add NormalizeMapKey DFG IR
Summary: [DFG] Add NormalizeMapKey DFG IR
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-11-21 02:34 PST by Yusuke Suzuki
Modified: 2017-11-26 23:52 PST (History)
10 users (show)

See Also:


Attachments
Patch (34.18 KB, patch)
2017-11-21 08:06 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (34.19 KB, patch)
2017-11-21 09:23 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-21 02:34:36 PST
...
Comment 1 Yusuke Suzuki 2017-11-21 08:06:09 PST
Created attachment 327410 [details]
Patch
Comment 2 EWS Watchlist 2017-11-21 08:51:22 PST
Comment on attachment 327410 [details]
Patch

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

New failing tests:
stress/proxy-own-keys.js.dfg-eager-no-cjit-validate
stress/proxy-own-keys.js.ftl-eager-no-cjit
stress/proxy-own-keys.js.ftl-eager
Comment 3 Yusuke Suzuki 2017-11-21 09:23:27 PST
Created attachment 327417 [details]
Patch
Comment 4 Saam Barati 2017-11-26 17:20:16 PST
Comment on attachment 327417 [details]
Patch

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

r=me

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1115
> +        forNode(node).makeHeapTop();

Is it worth doing constant folding here based on child1's type?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:5195
> +            // Give key and bucket's key are already normalized. So if 64-bit compare fails and one is not a cell, they're definitely not equal.

"Give key and bucket's key" => "The input key and bucket's key"

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8585
> +        m_out.appendTo(passThroughCase, continuation);
> +        ValueFromBlock fastResult = m_out.anchor(key);
> +        m_out.jump(continuation);

This doesn't need to be a block. You can make the upsilon at the top, and make everything that jumps to "passThroughCase" jump to continuation.
Comment 5 Yusuke Suzuki 2017-11-26 17:31:24 PST
Comment on attachment 327417 [details]
Patch

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

Thank you for your review!

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1115
>> +        forNode(node).makeHeapTop();
> 
> Is it worth doing constant folding here based on child1's type?

Sounds nice. I think large part of NormalizeMapKey will be converted to Identity in fixup phase. But mixed typed case is not handled well in fixup phase.
Introducing constant foldoing here could increase the coverage.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:5195
>> +            // Give key and bucket's key are already normalized. So if 64-bit compare fails and one is not a cell, they're definitely not equal.
> 
> "Give key and bucket's key" => "The input key and bucket's key"

Thanks, fixed.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:8585
>> +        m_out.jump(continuation);
> 
> This doesn't need to be a block. You can make the upsilon at the top, and make everything that jumps to "passThroughCase" jump to continuation.

Sounds nice. Fixed.
Comment 6 Yusuke Suzuki 2017-11-26 23:51:22 PST
Committed r225154: <https://trac.webkit.org/changeset/225154>
Comment 7 Radar WebKit Bug Importer 2017-11-26 23:52:29 PST
<rdar://problem/35693381>