Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1471)

Issue 1213803008: [turbofan] Right hand side of shifts needs ToUint32. (Closed)

Created:
5 years, 5 months ago by Benedikt Meurer
Modified:
5 years, 5 months ago
Reviewers:
Jarin
CC:
v8-dev
Base URL:
https://chromium.googlesource.com/v8/v8.git@master
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[turbofan] Right hand side of shifts needs ToUint32. Currently we lower shifts directly to machine operators, and add an appropriate Word32And to implement the & 0x1F operation on the right hand side required by the specification. However for Word32And we assume Int32 in simplified lowering, which is basically changes the right hand side bit interpretation for the shifts from Uint32 to Int32, which is obviously wrong. So now we represent that explicitly by proper simplified operators for the shifts, which are lowered to machine in simplified lowering. R=jarin@chromium.org Committed: https://crrev.com/5f288c201c37375ffe442f6e31ca6a4f36c3496f Cr-Commit-Position: refs/heads/master@{#29465}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+248 lines, -73 lines) Patch
M src/compiler/js-typed-lowering.h View 1 chunk +0 lines, -3 lines 0 comments Download
M src/compiler/js-typed-lowering.cc View 4 chunks +6 lines, -22 lines 0 comments Download
M src/compiler/opcodes.h View 1 chunk +3 lines, -0 lines 0 comments Download
M src/compiler/simplified-lowering.h View 3 chunks +3 lines, -2 lines 0 comments Download
M src/compiler/simplified-lowering.cc View 3 chunks +34 lines, -0 lines 0 comments Download
M src/compiler/simplified-operator.h View 1 chunk +3 lines, -0 lines 0 comments Download
M src/compiler/simplified-operator.cc View 1 chunk +29 lines, -26 lines 0 comments Download
M src/compiler/typer.cc View 1 chunk +15 lines, -0 lines 0 comments Download
M src/compiler/verifier.cc View 1 chunk +13 lines, -0 lines 0 comments Download
M test/cctest/compiler/test-js-typed-lowering.cc View 2 chunks +4 lines, -11 lines 0 comments Download
A test/mjsunit/compiler/regress-shift-left.js View 1 chunk +41 lines, -0 lines 0 comments Download
A test/mjsunit/compiler/regress-shift-right.js View 1 chunk +41 lines, -0 lines 0 comments Download
A test/mjsunit/compiler/regress-shift-right-logical.js View 1 chunk +41 lines, -0 lines 0 comments Download
M test/unittests/compiler/js-typed-lowering-unittest.cc View 6 chunks +6 lines, -9 lines 0 comments Download
M test/unittests/compiler/node-test-utils.h View 1 chunk +6 lines, -0 lines 0 comments Download
M test/unittests/compiler/node-test-utils.cc View 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 7 (1 generated)
Benedikt Meurer
5 years, 5 months ago (2015-07-03 10:52:20 UTC) #1
Benedikt Meurer
Hey Jaro, The fix for the simplified lowering bug that popped up today. Please take ...
5 years, 5 months ago (2015-07-03 10:53:03 UTC) #2
Jarin
lgtm
5 years, 5 months ago (2015-07-03 10:57:25 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1213803008/1
5 years, 5 months ago (2015-07-03 10:57:37 UTC) #5
commit-bot: I haz the power
Committed patchset #1 (id:1)
5 years, 5 months ago (2015-07-03 11:42:00 UTC) #6
commit-bot: I haz the power
5 years, 5 months ago (2015-07-03 11:42:09 UTC) #7
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/5f288c201c37375ffe442f6e31ca6a4f36c3496f
Cr-Commit-Position: refs/heads/master@{#29465}

Powered by Google App Engine
This is Rietveld 408576698