Chromium Code Reviews

Issue 1757213003: [turbofan] Do not produce 2 ops for Int32AddWithOverflow. (Closed)

Created:
4 years, 9 months ago by Mircea Trofin
Modified:
4 years, 9 months ago
Reviewers:
Benedikt Meurer, titzer
CC:
v8-reviews_googlegroups.com, v8-x87-ports_googlegroups.com
Base URL:
https://chromium.googlesource.com/v8/v8.git@master
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[turbofan] Do not produce 2 ops for Int32AddWithOverflow. It seems we produce both the usual add as well as the add with overflow, when we should only generate the overflow variant. This invalidates SSA assumptions in 2 tests (cctest/test-run-machops/RunInt32AddWithOverflowImm and cctest/test-run-machops/RunInt64AddWithOverflowImm). BUG= Committed: https://crrev.com/b9bfe33ee9357d361a283abde8c771dad1696820 Cr-Commit-Position: refs/heads/master@{#34507}

Patch Set 1 #

Total comments: 3

Patch Set 2 : #

Unified diffs Side-by-side diffs Stats (+1 line, -1 line)
M src/compiler/x64/instruction-selector-x64.cc View 1 chunk +1 line, -1 line 0 comments

Messages

Total messages: 16 (7 generated)
Mircea Trofin
I wonder why the v8_linux64_asan_rel run failed first, then, with no change, it passed. Looking ...
4 years, 9 months ago (2016-03-03 17:21:10 UTC) #3
Benedikt Meurer
https://codereview.chromium.org/1757213003/diff/1/src/compiler/ia32/instruction-selector-ia32.cc File src/compiler/ia32/instruction-selector-ia32.cc (right): https://codereview.chromium.org/1757213003/diff/1/src/compiler/ia32/instruction-selector-ia32.cc#newcode1332 src/compiler/ia32/instruction-selector-ia32.cc:1332: } else { This doesn't seem to be necessary, ...
4 years, 9 months ago (2016-03-03 17:48:09 UTC) #5
Mircea Trofin
https://codereview.chromium.org/1757213003/diff/1/src/compiler/ia32/instruction-selector-ia32.cc File src/compiler/ia32/instruction-selector-ia32.cc (right): https://codereview.chromium.org/1757213003/diff/1/src/compiler/ia32/instruction-selector-ia32.cc#newcode1332 src/compiler/ia32/instruction-selector-ia32.cc:1332: } else { On 2016/03/03 17:48:08, Benedikt Meurer wrote: ...
4 years, 9 months ago (2016-03-03 21:50:14 UTC) #6
Mircea Trofin
This is the other add with overflow.
4 years, 9 months ago (2016-03-04 16:08:33 UTC) #7
Mircea Trofin
4 years, 9 months ago (2016-03-04 19:34:26 UTC) #9
titzer
lgtm
4 years, 9 months ago (2016-03-04 19:38:00 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1757213003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1757213003/20001
4 years, 9 months ago (2016-03-04 19:40:56 UTC) #12
commit-bot: I haz the power
Committed patchset #2 (id:20001)
4 years, 9 months ago (2016-03-04 19:59:21 UTC) #14
commit-bot: I haz the power
4 years, 9 months ago (2016-03-04 20:00:52 UTC) #16
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/b9bfe33ee9357d361a283abde8c771dad1696820
Cr-Commit-Position: refs/heads/master@{#34507}

Powered by Google App Engine