Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2652893011:
[turbofan] Instance types don't change, so no need to watch for side effects. (Closed)
|
Created:
3 years, 10 months ago by Benedikt Meurer Modified:
3 years, 10 months ago Reviewers:
Michael Starzinger CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[turbofan] Instance types don't change, so no need to watch for side effects.
The instance type of an object cannot change, only the concrete map
might. So when searching for an instance type witness, we don't need
to pay attention to potentially side-effecting nodes.
R=mstarzinger@chromium.org
Review-Url: https://codereview.chromium.org/2652893011
Cr-Commit-Position: refs/heads/master@{#42699}
Committed: https://chromium.googlesource.com/v8/v8/+/7f441a5e1bb58c4d1101291d0afb9eb435433c88
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add guard #
Total comments: 2
Patch Set 3 : Address comment. #Messages
Total messages: 19 (11 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 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...
https://codereview.chromium.org/2652893011/diff/1/src/compiler/js-builtin-red... File src/compiler/js-builtin-reducer.cc (right): https://codereview.chromium.org/2652893011/diff/1/src/compiler/js-builtin-red... src/compiler/js-builtin-reducer.cc:915: // attention to potentially side-effecting nodes here. As discussed offline: The instance type of strings change in many places. Can we somehow guard this function from being misused (e.g. applied on string types)?
https://codereview.chromium.org/2652893011/diff/1/src/compiler/js-builtin-red... File src/compiler/js-builtin-reducer.cc (right): https://codereview.chromium.org/2652893011/diff/1/src/compiler/js-builtin-red... src/compiler/js-builtin-reducer.cc:915: // attention to potentially side-effecting nodes here. On 2017/01/26 12:45:38, Michael Starzinger wrote: > As discussed offline: The instance type of strings change in many places. Can we > somehow guard this function from being misused (e.g. applied on string types)? Done.
https://codereview.chromium.org/2652893011/diff/20001/src/compiler/js-builtin... File src/compiler/js-builtin-reducer.cc (right): https://codereview.chromium.org/2652893011/diff/20001/src/compiler/js-builtin... src/compiler/js-builtin-reducer.cc:917: DCHECK_LE(FIRST_NONSTRING_TYPE, instance_type); nit: I think it also doesn't hold for {FIXED_ARRAY} vs. {FIXED_DOUBLE_ARRAY} ... could we use {FIRST_JS_RECEIVER_TYPE} as a bound instead? Also comment should be more generic about "internal instance types".
LGTM once comment is addressed.
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...
Thanks, addressed. https://codereview.chromium.org/2652893011/diff/20001/src/compiler/js-builtin... File src/compiler/js-builtin-reducer.cc (right): https://codereview.chromium.org/2652893011/diff/20001/src/compiler/js-builtin... src/compiler/js-builtin-reducer.cc:917: DCHECK_LE(FIRST_NONSTRING_TYPE, instance_type); On 2017/01/26 13:09:49, Michael Starzinger wrote: > nit: I think it also doesn't hold for {FIXED_ARRAY} vs. {FIXED_DOUBLE_ARRAY} ... > could we use {FIRST_JS_RECEIVER_TYPE} as a bound instead? Also comment should be > more generic about "internal instance types". Done.
The CQ bit was unchecked by bmeurer@chromium.org
The CQ bit was checked by bmeurer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mstarzinger@chromium.org Link to the patchset: https://codereview.chromium.org/2652893011/#ps40001 (title: "Address comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485436615671280,
"parent_rev": "169b85673f2f8592c2bb89acd73f1cd25fbe58b5", "commit_rev":
"7f441a5e1bb58c4d1101291d0afb9eb435433c88"}
Message was sent while issue was closed.
Description was changed from ========== [turbofan] Instance types don't change, so no need to watch for side effects. The instance type of an object cannot change, only the concrete map might. So when searching for an instance type witness, we don't need to pay attention to potentially side-effecting nodes. R=mstarzinger@chromium.org ========== to ========== [turbofan] Instance types don't change, so no need to watch for side effects. The instance type of an object cannot change, only the concrete map might. So when searching for an instance type witness, we don't need to pay attention to potentially side-effecting nodes. R=mstarzinger@chromium.org Review-Url: https://codereview.chromium.org/2652893011 Cr-Commit-Position: refs/heads/master@{#42699} Committed: https://chromium.googlesource.com/v8/v8/+/7f441a5e1bb58c4d1101291d0afb9eb4354... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/v8/v8/+/7f441a5e1bb58c4d1101291d0afb9eb4354... |
