[turbofan] Track multiple maps for LoadElimination.

[turbofan] Track multiple maps for LoadElimination.

Store maps on the CheckMaps operator instead of burning inputs for
the individual maps. Use the same data structure (the ZoneHandleSet)
in the LoadElimination to track multiple maps per object.

BUG=v8:5267
R=jarin@chromium.org

Review-Url: https://codereview.chromium.org/2431563002
Cr-Commit-Position: refs/heads/master@{#42010}
Committed: https://chromium.googlesource.com/v8/v8/+/45aa13514b1997353a9d0bfbeaff9d4776c20d2a

[Benedikt Meurer, 2016-10-18 04:51:23]

Hey Jaro,

Here's the CL I mentioned yesterday.
Really needs a lot of polish.

-- Benedikt

[Jarin, 2016-11-16 14:54:41]

https://codereview.chromium.org/2431563002/diff/1/src/compiler/load-eliminati...
File src/compiler/load-elimination.cc (right):

https://codereview.chromium.org/2431563002/diff/1/src/compiler/load-eliminati...
src/compiler/load-elimination.cc:592: if (object_maps.contains(maps)) return
Replace(effect);
Still confused.

If I have MapCheck(m1, m2); MapCheck(m1); this would insert (m1, m2) to the
maps; then when processing MapCheck(m1), the check (m1, m2).contains(m1) would
pass, so we would remove MapCheck(m1), no? That would seem wrong.

https://codereview.chromium.org/2431563002/diff/1/src/compiler/load-eliminati...
File src/compiler/load-elimination.h (right):

https://codereview.chromium.org/2431563002/diff/1/src/compiler/load-eliminati...
src/compiler/load-elimination.h:197: ZoneMap<Node*, ZoneHandleSet<Map>>
info_for_node_;
I am super confused about what these sets mean. I would expect that if an object
o has a map-set m, then o's map must be in m.

However, what does empty map-set mean? Since we return empty set for unknown
nodes, does it mean we do not know anything. How do we distinguish from an
impossible set, e.g., if we we mapcheck m1 and then m2?

[Benedikt Meurer, 2016-12-21 08:28:00]

The CQ bit was checked by bmeurer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-21 08:28:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Benedikt Meurer, 2016-12-21 08:36:49]

https://codereview.chromium.org/2431563002/diff/1/src/compiler/load-eliminati...
File src/compiler/load-elimination.cc (right):

https://codereview.chromium.org/2431563002/diff/1/src/compiler/load-eliminati...
src/compiler/load-elimination.cc:592: if (object_maps.contains(maps)) return
Replace(effect);
Done.

[commit-bot: I haz the power, 2016-12-21 08:49:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-21 08:49:47]

Dry run: Try jobs failed on following builders:
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/19836)
  v8_win64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng_triggered/b...)

[Benedikt Meurer, 2016-12-21 12:09:10]

The CQ bit was checked by bmeurer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-21 12:09:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-21 12:37:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-21 12:37:26]

Dry run: This issue passed the CQ dry run.

[Benedikt Meurer, 2016-12-21 18:54:08]

The CQ bit was checked by bmeurer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-21 18:54:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-21 19:19:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-21 19:19:08]

Dry run: This issue passed the CQ dry run.

[Benedikt Meurer, 2016-12-22 08:27:37]

The CQ bit was checked by bmeurer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-22 08:27:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Benedikt Meurer, 2016-12-22 08:28:12]

Hey Jaro,

How do you like this version?
Please take a look.

Thanks,
Benedikt

[commit-bot: I haz the power, 2016-12-22 08:52:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-22 08:52:41]

Dry run: This issue passed the CQ dry run.

[Jarin, 2017-01-01 19:27:35]

lgtm, although ZoneHandleSet still looks like premature optimization to me. Any
numbers to support the complexity?

https://codereview.chromium.org/2431563002/diff/80001/src/compiler/js-native-...
File src/compiler/js-native-context-specialization.cc (right):

https://codereview.chromium.org/2431563002/diff/80001/src/compiler/js-native-...
src/compiler/js-native-context-specialization.cc:1655: // TODO(turbofan): This
could be more efficient.
Remove the comment.

[Benedikt Meurer, 2017-01-02 05:14:01]

On 2017/01/01 19:27:35, Jarin wrote:
> lgtm, although ZoneHandleSet still looks like premature optimization to me.
Any
> numbers to support the complexity?

Over 90% of the property accesses are monomorphic (on Octane+Kraken+SunSpider).

What would you suggest as simple solution? A std::set?

[Benedikt Meurer, 2017-01-02 12:31:55]

Description was changed from

==========
[WIP] Tracking multiple maps for LoadElimination.

R=jarin@chromium.org
==========

to

==========
[turbofan] Track multiple maps for LoadElimination.

Store maps on the CheckMaps operator instead of burning inputs for
the individual maps. Use the same data structure (the ZoneHandleSet)
in the LoadElimination to track multiple maps per object.

BUG=v8:5267
R=jarin@chromium.org
==========

[Benedikt Meurer, 2017-01-02 12:32:58]

https://codereview.chromium.org/2431563002/diff/80001/src/compiler/js-native-...
File src/compiler/js-native-context-specialization.cc (right):

https://codereview.chromium.org/2431563002/diff/80001/src/compiler/js-native-...
src/compiler/js-native-context-specialization.cc:1655: // TODO(turbofan): This
could be more efficient.
On 2017/01/01 19:27:35, Jarin wrote:
> Remove the comment.

Done.

[Benedikt Meurer, 2017-01-02 12:33:20]

The CQ bit was checked by bmeurer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-02 12:33:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Benedikt Meurer, 2017-01-02 12:35:07]

The CQ bit was unchecked by bmeurer@chromium.org

[Benedikt Meurer, 2017-01-02 12:35:12]

The CQ bit was checked by bmeurer@chromium.org

[Benedikt Meurer, 2017-01-02 12:35:12]

The patchset sent to the CQ was uploaded after l-g-t-m from jarin@chromium.org
Link to the patchset: https://codereview.chromium.org/2431563002/#ps100001
(title: "REBASE. Address comment.")

[commit-bot: I haz the power, 2017-01-02 12:35:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-02 13:03:03]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1483360512204170,
"parent_rev": "09cb6efd588521ef95c6fa7ba2c3e18331648ea0", "commit_rev":
"45aa13514b1997353a9d0bfbeaff9d4776c20d2a"}

[commit-bot: I haz the power, 2017-01-02 13:03:07]

Description was changed from

==========
[turbofan] Track multiple maps for LoadElimination.

Store maps on the CheckMaps operator instead of burning inputs for
the individual maps. Use the same data structure (the ZoneHandleSet)
in the LoadElimination to track multiple maps per object.

BUG=v8:5267
R=jarin@chromium.org
==========

to

==========
[turbofan] Track multiple maps for LoadElimination.

Store maps on the CheckMaps operator instead of burning inputs for
the individual maps. Use the same data structure (the ZoneHandleSet)
in the LoadElimination to track multiple maps per object.

BUG=v8:5267
R=jarin@chromium.org

Review-Url: https://codereview.chromium.org/2431563002
Cr-Commit-Position: refs/heads/master@{#42010}
Committed:
https://chromium.googlesource.com/v8/v8/+/45aa13514b1997353a9d0bfbeaff9d4776c...
==========

[commit-bot: I haz the power, 2017-01-02 13:03:07]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/v8/v8/+/45aa13514b1997353a9d0bfbeaff9d4776c...