Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2669203005:
[arm64][turbofan] Fix add+shr for big shift values. (Closed)
Description[arm64][turbofan] Fix add+shr for big shift values.
Arm64 compiles "x +_64 (y >> shift)" into a single instruction if
"shift" is a constant. The code generator expects that "shift" is a
32 bit constant. however, TurboFan can also pass in a 64 bit constant,
which caused a crash in the code generator.
With this CL we cast the constant of TurboFan to an int in the
instruction selector and thereby satisfy the assumption of the code
generator. This should be correct since the code generator anyways cast
the "shift" to an int5 or int6 eventually.
R=v8-arm-ports@googlegroups.com
BUG=v8:5923
Review-Url: https://codereview.chromium.org/2669203005
Cr-Commit-Position: refs/heads/master@{#43036}
Committed: https://chromium.googlesource.com/v8/v8/+/ed6e28d2ad00326244abc6988783a5e04b873ef4
Patch Set 1 #
Total comments: 2
Patch Set 2 : Use only the last 6 bit of the shift. #Patch Set 3 : Fixed an issue with the test. #Patch Set 4 : Adjusted a unittest #Patch Set 5 : Only consider the last 6 bits of the shift immediate #
Messages
Total messages: 37 (26 generated)
The CQ bit was checked by ahaas@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: Try jobs failed on following builders: v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/22016) v8_win64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng_triggered/b...)
martyn.capewell@arm.com changed reviewers: + martyn.capewell@arm.com
https://codereview.chromium.org/2669203005/diff/1/src/compiler/arm64/instruct... File src/compiler/arm64/instruction-selector-arm64.cc (right): https://codereview.chromium.org/2669203005/diff/1/src/compiler/arm64/instruct... src/compiler/arm64/instruction-selector-arm64.cc:440: g.UseImmediate(static_cast<int>(m_shift.right().Value())); I think this cast is implementation defined; the input is out of range of the int, as that's the problem you're trying to solve. If we can assume the shift is positive, a uint32_t cast would work.
The CQ bit was checked by ahaas@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/2669203005/diff/1/src/compiler/arm64/instruct... File src/compiler/arm64/instruction-selector-arm64.cc (right): https://codereview.chromium.org/2669203005/diff/1/src/compiler/arm64/instruct... src/compiler/arm64/instruction-selector-arm64.cc:440: g.UseImmediate(static_cast<int>(m_shift.right().Value())); On 2017/02/03 at 11:44:37, martyn.capewell wrote: > I think this cast is implementation defined; the input is out of range of the int, as that's the problem you're trying to solve. If we can assume the shift is positive, a uint32_t cast would work. Thanks. a uint32_t cast would not help either because g.UseImmediate takes an int parameter. I apply a bit mask now to make sure that the input value is within int range. This is possible because shift is restricted to 6 bits anyways.
The CQ bit was checked by ahaas@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: Try jobs failed on following builders: v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_arm64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng_trigg...)
lgtm
The CQ bit was checked by ahaas@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: Try jobs failed on following builders: v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_arm64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng_trigg...)
The CQ bit was checked by ahaas@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...
On 2017/02/03 at 15:15:50, martyn.capewell wrote: > lgtm I had to adjust unittests. They expected that shift immediates with more than 6 bits are stored in the instruction sequence, especially negative shift immediates. This, however, has changed in this CL, because now only 6 bit immediates are stored in the instruction sequence.
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 ahaas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from martyn.capewell@arm.com Link to the patchset: https://codereview.chromium.org/2669203005/#ps80001 (title: "Only consider the last 6 bits of the shift immediate")
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
Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/34179)
ahaas@chromium.org changed reviewers: + bmeurer@chromium.org
LGTM (rubberstamped)
LGTM (rubberstamped)
The CQ bit was checked by ahaas@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": 80001, "attempt_start_ts": 1486553395643250,
"parent_rev": "a1bba7fe3b9e52a5c5f556060b563427404231be", "commit_rev":
"ed6e28d2ad00326244abc6988783a5e04b873ef4"}
Message was sent while issue was closed.
Description was changed from ========== [arm64][turbofan] Fix add+shr for big shift values. Arm64 compiles "x +_64 (y >> shift)" into a single instruction if "shift" is a constant. The code generator expects that "shift" is a 32 bit constant. however, TurboFan can also pass in a 64 bit constant, which caused a crash in the code generator. With this CL we cast the constant of TurboFan to an int in the instruction selector and thereby satisfy the assumption of the code generator. This should be correct since the code generator anyways cast the "shift" to an int5 or int6 eventually. R=v8-arm-ports@googlegroups.com BUG=v8:5923 ========== to ========== [arm64][turbofan] Fix add+shr for big shift values. Arm64 compiles "x +_64 (y >> shift)" into a single instruction if "shift" is a constant. The code generator expects that "shift" is a 32 bit constant. however, TurboFan can also pass in a 64 bit constant, which caused a crash in the code generator. With this CL we cast the constant of TurboFan to an int in the instruction selector and thereby satisfy the assumption of the code generator. This should be correct since the code generator anyways cast the "shift" to an int5 or int6 eventually. R=v8-arm-ports@googlegroups.com BUG=v8:5923 Review-Url: https://codereview.chromium.org/2669203005 Cr-Commit-Position: refs/heads/master@{#43036} Committed: https://chromium.googlesource.com/v8/v8/+/ed6e28d2ad00326244abc6988783a5e04b8... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/v8/v8/+/ed6e28d2ad00326244abc6988783a5e04b8... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
