Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2164253002:
[turbofan] New GraphReducer based LoadElimination. (Closed)
|
Created:
4 years, 5 months ago by Benedikt Meurer Modified:
4 years, 5 months ago Reviewers:
Jarin CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[turbofan] New GraphReducer based LoadElimination.
Turn the LoadElimination into a proper graph Reducer so that it can run
together with ValueNumbering and RedundancyElimination to a fixpoint
for maximum load/check elimination. This also adds initial support for
eliminating redundant LoadElement/StoreElement nodes.
BUG=v8:4930, v8:5141
Committed: https://crrev.com/a2ad4c8f62a4c42e07df9b084675f7ebc03c1312
Cr-Commit-Position: refs/heads/master@{#38015}
Patch Set 1 : Initial version. #Patch Set 2 : Simplify pipeline a bit. #Patch Set 3 : Add unittests for the LoadElimination. #Patch Set 4 : Teach alias analysis about FinishRegion. #
Total comments: 6
Patch Set 5 : Address comments. REBASE. #
Messages
Total messages: 34 (28 generated)
The CQ bit was checked by bmeurer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bmeurer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:1) has been deleted
Description was changed from ========== [WIP] New LoadElimination as reducer (with support for elements). ========== to ========== [turbofan] New GraphReducer based LoadElimination. Turn the LoadElimination into a proper graph Reducer so that it can run together with ValueNumbering and RedundancyElimination to a fixpoint for maximum load/check elimination. This also adds initial support for eliminating redundant LoadElement/StoreElement nodes. ==========
bmeurer@chromium.org changed reviewers: + jarin@chromium.org
Description was changed from ========== [turbofan] New GraphReducer based LoadElimination. Turn the LoadElimination into a proper graph Reducer so that it can run together with ValueNumbering and RedundancyElimination to a fixpoint for maximum load/check elimination. This also adds initial support for eliminating redundant LoadElement/StoreElement nodes. ========== to ========== [turbofan] New GraphReducer based LoadElimination. Turn the LoadElimination into a proper graph Reducer so that it can run together with ValueNumbering and RedundancyElimination to a fixpoint for maximum load/check elimination. This also adds initial support for eliminating redundant LoadElement/StoreElement nodes. BUG=v8:4930,v8:5141 ==========
Hey Jaro, I think this is ready for early feedback. Will still need to do some unittests, and maybe some cleanups. Please take a look. Thanks, Benedikt
The CQ bit was checked by bmeurer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bmeurer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bmeurer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This looks awesome! lgtm. https://codereview.chromium.org/2164253002/diff/80001/src/compiler/load-elimi... File src/compiler/load-elimination.cc (right): https://codereview.chromium.org/2164253002/diff/80001/src/compiler/load-elimi... src/compiler/load-elimination.cc:92: if (MayAlias(object, element.object) && MayAlias(index, element.index)) { Please, remove the MayAlias for the index. This seems to work for now, but only because MayAlias is dumb. If we made MayAlias a bit smarter (e.g., by doing aliasing for heap constants), this could blow up. https://codereview.chromium.org/2164253002/diff/80001/src/compiler/load-elimi... src/compiler/load-elimination.cc:416: // the information from the loop entry edge. Replace the comment with one that makes sense :-) https://codereview.chromium.org/2164253002/diff/80001/src/compiler/load-elimi... File src/compiler/load-elimination.h (right): https://codereview.chromium.org/2164253002/diff/80001/src/compiler/load-elimi... src/compiler/load-elimination.h:107: ZoneMap<Node*, Node*> info_for_node_; Please, use the id on the left-hand side.
https://codereview.chromium.org/2164253002/diff/80001/src/compiler/load-elimi... File src/compiler/load-elimination.cc (right): https://codereview.chromium.org/2164253002/diff/80001/src/compiler/load-elimi... src/compiler/load-elimination.cc:92: if (MayAlias(object, element.object) && MayAlias(index, element.index)) { On 2016/07/25 11:22:27, Jarin wrote: > Please, remove the MayAlias for the index. This seems to work for now, but only > because MayAlias is dumb. If we made MayAlias a bit smarter (e.g., by doing > aliasing for heap constants), this could blow up. Done. https://codereview.chromium.org/2164253002/diff/80001/src/compiler/load-elimi... src/compiler/load-elimination.cc:416: // the information from the loop entry edge. On 2016/07/25 11:22:27, Jarin wrote: > Replace the comment with one that makes sense :-) Done. https://codereview.chromium.org/2164253002/diff/80001/src/compiler/load-elimi... File src/compiler/load-elimination.h (right): https://codereview.chromium.org/2164253002/diff/80001/src/compiler/load-elimi... src/compiler/load-elimination.h:107: ZoneMap<Node*, Node*> info_for_node_; We cannot fix this currently, as we need the object for the MayAlias and MustAlias in Kill/Lookup.
The CQ bit was checked by bmeurer@chromium.org
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/2164253002/#ps100001 (title: "Address comments. REBASE.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [turbofan] New GraphReducer based LoadElimination. Turn the LoadElimination into a proper graph Reducer so that it can run together with ValueNumbering and RedundancyElimination to a fixpoint for maximum load/check elimination. This also adds initial support for eliminating redundant LoadElement/StoreElement nodes. BUG=v8:4930,v8:5141 ========== to ========== [turbofan] New GraphReducer based LoadElimination. Turn the LoadElimination into a proper graph Reducer so that it can run together with ValueNumbering and RedundancyElimination to a fixpoint for maximum load/check elimination. This also adds initial support for eliminating redundant LoadElement/StoreElement nodes. BUG=v8:4930,v8:5141 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [turbofan] New GraphReducer based LoadElimination. Turn the LoadElimination into a proper graph Reducer so that it can run together with ValueNumbering and RedundancyElimination to a fixpoint for maximum load/check elimination. This also adds initial support for eliminating redundant LoadElement/StoreElement nodes. BUG=v8:4930,v8:5141 ========== to ========== [turbofan] New GraphReducer based LoadElimination. Turn the LoadElimination into a proper graph Reducer so that it can run together with ValueNumbering and RedundancyElimination to a fixpoint for maximum load/check elimination. This also adds initial support for eliminating redundant LoadElement/StoreElement nodes. BUG=v8:4930,v8:5141 Committed: https://crrev.com/a2ad4c8f62a4c42e07df9b084675f7ebc03c1312 Cr-Commit-Position: refs/heads/master@{#38015} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/a2ad4c8f62a4c42e07df9b084675f7ebc03c1312 Cr-Commit-Position: refs/heads/master@{#38015} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
