Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2691103008:
[gcmole] Fixes for unreachable code (Closed)
|
Created:
3 years, 10 months ago by Clemens Hammacher Modified:
3 years, 10 months ago CC:
Michael Achenbach, Michael Starzinger, v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[gcmole] Fixes for unreachable code
Enforce the invariant that unreachable implicates an empty live set and
fix the implementation of |= and &= operator.
This is a fix-up for http://crrev.com/2694103005.
R=vegorov@chromium.org
CC=mstarzinger@chromium.org, machenbach@chromium.org
BUG=v8:5970
Review-Url: https://codereview.chromium.org/2691103008
Cr-Commit-Position: refs/heads/master@{#43251}
Committed: https://chromium.googlesource.com/v8/v8/+/4b0edcf7e06169b6daf0c18c14c42265a3024006
Patch Set 1 #
Total comments: 5
Patch Set 2 : Add comment #
Depends on Patchset: Messages
Total messages: 27 (16 generated)
The CQ bit was checked by clemensh@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.
vegorov@google.com changed reviewers: + vegorov@google.com
LGTM if the conjunction code is fixed. https://codereview.chromium.org/2691103008/diff/1/tools/gcmole/gcmole.cc File tools/gcmole/gcmole.cc (right): https://codereview.chromium.org/2691103008/diff/1/tools/gcmole/gcmole.cc#newc... tools/gcmole/gcmole.cc:433: size_t size = std::max(live_.size(), o.live_.size()); I think now that unreachable is handled above it would be correctly to use min(...) here and < below because whichever of two (|this| or |o|) is longer its gonna be conjugated with the tail of the smaller one which is all false and disappear. https://codereview.chromium.org/2691103008/diff/1/tools/gcmole/gcmole.cc#newc... tools/gcmole/gcmole.cc:436: if (live_[i] && (i >= o.live_.size() || !o.live_[i])) live_[i] = false; this should just be live_[i] = live_[i] && o.live_[i] because size would be the minimum of sizes.
https://codereview.chromium.org/2691103008/diff/1/tools/gcmole/gcmole.cc File tools/gcmole/gcmole.cc (right): https://codereview.chromium.org/2691103008/diff/1/tools/gcmole/gcmole.cc#newc... tools/gcmole/gcmole.cc:433: size_t size = std::max(live_.size(), o.live_.size()); On 2017/02/16 at 11:53:00, Vyacheslav Egorov (Google) wrote: > I think now that unreachable is handled above it would be correctly to use min(...) here and < below because whichever of two (|this| or |o|) is longer its gonna be conjugated with the tail of the smaller one which is all false and disappear. What if |o| > |this| and the tail of o is all false. Wouldn't we want to resize |o| to also contain those false values? I think that's relevant for the Equals operator in the case that one in unreachable and the other is not. We need to distinguish between false values and non-existing values, no?
https://codereview.chromium.org/2691103008/diff/1/tools/gcmole/gcmole.cc File tools/gcmole/gcmole.cc (right): https://codereview.chromium.org/2691103008/diff/1/tools/gcmole/gcmole.cc#newc... tools/gcmole/gcmole.cc:433: size_t size = std::max(live_.size(), o.live_.size()); I see what you mean. Yeah, this subtle. Might warrant a comment here.
https://codereview.chromium.org/2691103008/diff/1/tools/gcmole/gcmole.cc File tools/gcmole/gcmole.cc (right): https://codereview.chromium.org/2691103008/diff/1/tools/gcmole/gcmole.cc#newc... tools/gcmole/gcmole.cc:433: size_t size = std::max(live_.size(), o.live_.size()); On 2017/02/16 at 12:17:31, Vyacheslav Egorov (Google) wrote: > I see what you mean. Yeah, this subtle. Might warrant a comment here. Added comment.
The CQ bit was checked by clemensh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vegorov@google.com Link to the patchset: https://codereview.chromium.org/2691103008/#ps20001 (title: "Add comment")
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-v8-committers". Note that this has nothing to do with OWNERS files.
On 2017/02/16 at 13:13:22, commit-bot wrote: > No L-G-T-M from a valid reviewer yet. > CQ run can only be started by full committers or once the patch has > received an L-G-T-M from a full committer. > Even if an L-G-T-M may have been provided, it was from a non-committer, > _not_ a full super star committer. > Committers are members of the group "project-v8-committers". > Note that this has nothing to do with OWNERS files. @machenbach / @mstarzinger: Can one of you please LGTM?
The CQ bit was checked by clemensh@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...
machenbach@chromium.org changed reviewers: + machenbach@chromium.org
lgtm stamp
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 clemensh@chromium.org
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": 20001, "attempt_start_ts": 1487254295595740,
"parent_rev": "e2b83fbbfdf403903d3b4e5bd9276d38151789c3", "commit_rev":
"4b0edcf7e06169b6daf0c18c14c42265a3024006"}
Message was sent while issue was closed.
Description was changed from ========== [gcmole] Fixes for unreachable code Enforce the invariant that unreachable implicates an empty live set and fix the implementation of |= and &= operator. This is a fix-up for http://crrev.com/2694103005. R=vegorov@chromium.org CC=mstarzinger@chromium.org, machenbach@chromium.org BUG=v8:5970 ========== to ========== [gcmole] Fixes for unreachable code Enforce the invariant that unreachable implicates an empty live set and fix the implementation of |= and &= operator. This is a fix-up for http://crrev.com/2694103005. R=vegorov@chromium.org CC=mstarzinger@chromium.org, machenbach@chromium.org BUG=v8:5970 Review-Url: https://codereview.chromium.org/2691103008 Cr-Commit-Position: refs/heads/master@{#43251} Committed: https://chromium.googlesource.com/v8/v8/+/4b0edcf7e06169b6daf0c18c14c42265a30... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/v8/v8/+/4b0edcf7e06169b6daf0c18c14c42265a30... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
