Bug 159859 - DFG CSE should know how to decay a MultiGetByOffset
Summary: DFG CSE should know how to decay a MultiGetByOffset
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords: InRadar
Depends on: 159858
Blocks:
  Show dependency treegraph
 
Reported: 2016-07-16 15:37 PDT by Filip Pizlo
Modified: 2018-04-24 11:56 PDT (History)
6 users (show)

See Also:


Attachments
the patch (7.95 KB, patch)
2018-04-24 08:48 PDT, Filip Pizlo
keith_miller: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2016-07-16 15:37:52 PDT
This probably means teaching Node::remove() how to decay a MultiGetByOffset, which implies passing Graph& to remove() so that it can create a StructureSet.
Comment 1 Saam Barati 2016-10-23 12:29:04 PDT
Is MultiGetByOffset not dominated by a StructureCheck the way GetByOffset is?
Comment 2 Filip Pizlo 2016-10-23 13:02:26 PDT
(In reply to comment #1)
> Is MultiGetByOffset not dominated by a StructureCheck the way GetByOffset is?

It's not dominated by a CheckStructure.  The MultiGetByOffset does its own structure check.

We want one switch statement that both checks that we have the structure we want and loads the value.  CheckStructure with multiple structures in its set will be a switch and MultiGetByOffset is always a switch (unless we failed to strength-reduce a one-case MultiGetByOffset), so that would be two switches.  That's not cool, because we can't guarantee that B3 will jump-thread or tail-dup two switches in a row.  So, MultiGetByOffset is its own check.
Comment 3 Saam Barati 2016-10-28 02:31:48 PDT
(In reply to comment #2)
> (In reply to comment #1)
> > Is MultiGetByOffset not dominated by a StructureCheck the way GetByOffset is?
> 
> It's not dominated by a CheckStructure.  The MultiGetByOffset does its own
> structure check.
> 
> We want one switch statement that both checks that we have the structure we
> want and loads the value.  CheckStructure with multiple structures in its
> set will be a switch and MultiGetByOffset is always a switch (unless we
> failed to strength-reduce a one-case MultiGetByOffset), so that would be two
> switches.  That's not cool, because we can't guarantee that B3 will
> jump-thread or tail-dup two switches in a row.  So, MultiGetByOffset is its
> own check.

Gotcha. I'll work on fixing this.
Comment 4 Filip Pizlo 2018-04-24 08:48:50 PDT
Created attachment 338653 [details]
the patch
Comment 5 Keith Miller 2018-04-24 11:49:59 PDT
Comment on attachment 338653 [details]
the patch

r=me.
Comment 6 Filip Pizlo 2018-04-24 11:55:02 PDT
Landed in https://trac.webkit.org/changeset/230964/webkit
Comment 7 Radar WebKit Bug Importer 2018-04-24 11:56:26 PDT
<rdar://problem/39692355>