Chromium Code Reviews
|
|
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 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 : #Messages
Total messages: 16 (7 generated)
Description was changed from ========== Do not produce 2 ops for Int32AddWithOverflow BUG= ========== to ========== 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= ==========
mtrofin@chromium.org changed reviewers: + bmeurer@chromium.org
I wonder why the v8_linux64_asan_rel run failed first, then, with no change, it passed. Looking at the failures (heap buffer overflow) I wonder if this change uncovered an issue. I will investigate, but first I would like feedback if the change is the correct one for the double add issue in the 2 cctests mentioned. Thanks!
Description was changed from ========== 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= ========== to ========== [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= ==========
https://codereview.chromium.org/1757213003/diff/1/src/compiler/ia32/instructi... File src/compiler/ia32/instruction-selector-ia32.cc (right): https://codereview.chromium.org/1757213003/diff/1/src/compiler/ia32/instructi... src/compiler/ia32/instruction-selector-ia32.cc:1332: } else { This doesn't seem to be necessary, since there's a return above. Please keep this version with the return for consistency with the rest of the file. https://codereview.chromium.org/1757213003/diff/1/src/compiler/x64/instructio... File src/compiler/x64/instruction-selector-x64.cc (right): https://codereview.chromium.org/1757213003/diff/1/src/compiler/x64/instructio... src/compiler/x64/instruction-selector-x64.cc:1789: } else { Please leave the version w/o the else, and instead add return above similar to the rest of the file.
https://codereview.chromium.org/1757213003/diff/1/src/compiler/ia32/instructi... File src/compiler/ia32/instruction-selector-ia32.cc (right): https://codereview.chromium.org/1757213003/diff/1/src/compiler/ia32/instructi... src/compiler/ia32/instruction-selector-ia32.cc:1332: } else { On 2016/03/03 17:48:08, Benedikt Meurer wrote: > This doesn't seem to be necessary, since there's a return above. Please keep > this version with the return for consistency with the rest of the file. Ugh... true. Haven't noticed the return.
This is the other add with overflow.
mtrofin@chromium.org changed reviewers: + titzer@chromium.org
lgtm
The CQ bit was checked by mtrofin@chromium.org
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
Message was sent while issue was closed.
Description was changed from ========== [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= ========== to ========== [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= ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [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= ========== to ========== [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} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/b9bfe33ee9357d361a283abde8c771dad1696820 Cr-Commit-Position: refs/heads/master@{#34507} |
