Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1503173002:
MIPS64: [turbofan] Changed TruncateFloat64ToInt64 to TryTruncateFloat64ToInt64 (Closed)
|
Created:
5 years ago by ivica.bogosavljevic Modified:
5 years ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionMIPS64: [turbofan] Changed TruncateFloat64ToInt64 to TryTruncateFloat64ToInt64
Port 95844d94f3336489403c7f2d70c6ea01a0cf3002
Original commit message:
The new operator provides a second output which indicates whether the
conversion from float64 to int64 was successful or not. The second
output returns 0 if the conversion fails. If the conversion succeeds,
then the second output is differs from 0.
The second output can be ignored, which means that the operator can be
used the same way as the original operator.
I implemented the new operator on x64 and arm64. @v8-mips-ports and
@v8-ppc-ports, can you please take care of the mips64 and ppc64
implementation of the second output?
BUG=
TEST=cctest/test-run-machops/RunTryTruncateFloat64ToInt64WithCheck
Committed: https://crrev.com/80f2a6390cca5fe4b2b2f97cf106b61126d9f741
Cr-Commit-Position: refs/heads/master@{#32664}
Patch Set 1 #
Total comments: 1
Messages
Total messages: 26 (12 generated)
Have you run tests? Please add ahaas to the reviewers. And please edit the description as Paul suggested: 'and the second line should be 'Port https://codereview.chromium.org/1495213003/'' and put a dot to the end of the 1st description sentence :)
On 2015/12/07 19:08:37, balazs.kilvady wrote: > Have you run tests? > Please add ahaas to the reviewers. > And please edit the description as Paul suggested: 'and the second line should > be 'Port https://codereview.chromium.org/1495213003/ and put a dot to the end > of the 1st description sentence :) I executed all the tests from the test-machops-suite on board and v8 simulator and got all pass.
Description was changed from ========== MIPS64: Implement TryTruncateFloat64ToInt64 operator Implements TryTruncateFloat64ToInt64 on MIPS64 as done for other architectures in https://codereview.chromium.org/1495213003/ BUG= ========== to ========== MIPS64: Implement TryTruncateFloat64ToInt64 operator Ports TryTruncateFloat64ToInt64 on MIPS64 as done for other architectures in https://codereview.chromium.org/1495213003/. BUG= ==========
Description was changed from ========== MIPS64: Implement TryTruncateFloat64ToInt64 operator Ports TryTruncateFloat64ToInt64 on MIPS64 as done for other architectures in https://codereview.chromium.org/1495213003/. BUG= ========== to ========== MIPS64: Port TryTruncateFloat64ToInt64 operator Ports TryTruncateFloat64ToInt64 on MIPS64 as done for other architectures in https://codereview.chromium.org/1495213003/. BUG= ==========
ivica.bogosavljevic@imgtec.com changed reviewers: + ahaas@chromium.org
lgtm with nits: Please add ahaas to the reviewers. And please edit the description as Paul suggested: 'and the second line should be 'Port https://codereview.chromium.org/1495213003/ and put a dot to the end of the 1st description sentence :) And please add a TEST=cctest/test-run-machops/RunTryTruncateFloat64ToInt64WithCheck line under BUG=
On 2015/12/07 19:24:29, balazs.kilvady wrote: lgtm with nits: Please add ahaas to the reviewers. And please edit the description as Paul suggested: 'and the second line should be 'Port 95844d94f3336489403c7f2d70c6ea01a0cf3002' and put a dot to the end of the 1st description sentence :) And please add a TEST=cctest/test-run-machops/RunTryTruncateFloat64ToInt64WithCheck line under BUG=
Correction, since 'ahass' CL has landed, we must put the commit ID in the 'Port' line on 2nd line. 'Port 95844d94f3336489403c7f2d70c6ea01a0cf3002'
Description was changed from ========== MIPS64: Port TryTruncateFloat64ToInt64 operator Ports TryTruncateFloat64ToInt64 on MIPS64 as done for other architectures in https://codereview.chromium.org/1495213003/. BUG= ========== to ========== MIPS64: Port TryTruncateFloat64ToInt64 operator Port 95844d94f3336489403c7f2d70c6ea01a0cf3002 BUG= TEST=cctest/test-run-machops/RunTryTruncateFloat64ToInt64WithCheck ==========
The CQ bit was checked by ivica.bogosavljevic@imgtec.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1503173002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1503173002/1
Description was changed from ========== MIPS64: Port TryTruncateFloat64ToInt64 operator Port 95844d94f3336489403c7f2d70c6ea01a0cf3002 BUG= TEST=cctest/test-run-machops/RunTryTruncateFloat64ToInt64WithCheck ========== to ========== MIPS64: [turbofan] Changed TruncateFloat64ToInt64 to TryTruncateFloat64ToInt64 Port 95844d94f3336489403c7f2d70c6ea01a0cf3002 BUG= TEST=cctest/test-run-machops/RunTryTruncateFloat64ToInt64WithCheck ==========
Description was changed from ========== MIPS64: [turbofan] Changed TruncateFloat64ToInt64 to TryTruncateFloat64ToInt64 Port 95844d94f3336489403c7f2d70c6ea01a0cf3002 BUG= TEST=cctest/test-run-machops/RunTryTruncateFloat64ToInt64WithCheck ========== to ========== MIPS64: [turbofan] Changed TruncateFloat64ToInt64 to TryTruncateFloat64ToInt64 Port 95844d94f3336489403c7f2d70c6ea01a0cf3002 Original commit message: The new operator provides a second output which indicates whether the conversion from float64 to int64 was successful or not. The second output returns 0 if the conversion fails. If the conversion succeeds, then the second output is differs from 0. The second output can be ignored, which means that the operator can be used the same way as the original operator. I implemented the new operator on x64 and arm64. @v8-mips-ports and @v8-ppc-ports, can you please take care of the mips64 and ppc64 implementation of the second output? BUG= TEST=cctest/test-run-machops/RunTryTruncateFloat64ToInt64WithCheck ==========
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 ivica.bogosavljevic@imgtec.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1503173002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1503173002/1
Message was sent while issue was closed.
Description was changed from ========== MIPS64: [turbofan] Changed TruncateFloat64ToInt64 to TryTruncateFloat64ToInt64 Port 95844d94f3336489403c7f2d70c6ea01a0cf3002 Original commit message: The new operator provides a second output which indicates whether the conversion from float64 to int64 was successful or not. The second output returns 0 if the conversion fails. If the conversion succeeds, then the second output is differs from 0. The second output can be ignored, which means that the operator can be used the same way as the original operator. I implemented the new operator on x64 and arm64. @v8-mips-ports and @v8-ppc-ports, can you please take care of the mips64 and ppc64 implementation of the second output? BUG= TEST=cctest/test-run-machops/RunTryTruncateFloat64ToInt64WithCheck ========== to ========== MIPS64: [turbofan] Changed TruncateFloat64ToInt64 to TryTruncateFloat64ToInt64 Port 95844d94f3336489403c7f2d70c6ea01a0cf3002 Original commit message: The new operator provides a second output which indicates whether the conversion from float64 to int64 was successful or not. The second output returns 0 if the conversion fails. If the conversion succeeds, then the second output is differs from 0. The second output can be ignored, which means that the operator can be used the same way as the original operator. I implemented the new operator on x64 and arm64. @v8-mips-ports and @v8-ppc-ports, can you please take care of the mips64 and ppc64 implementation of the second output? BUG= TEST=cctest/test-run-machops/RunTryTruncateFloat64ToInt64WithCheck ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== MIPS64: [turbofan] Changed TruncateFloat64ToInt64 to TryTruncateFloat64ToInt64 Port 95844d94f3336489403c7f2d70c6ea01a0cf3002 Original commit message: The new operator provides a second output which indicates whether the conversion from float64 to int64 was successful or not. The second output returns 0 if the conversion fails. If the conversion succeeds, then the second output is differs from 0. The second output can be ignored, which means that the operator can be used the same way as the original operator. I implemented the new operator on x64 and arm64. @v8-mips-ports and @v8-ppc-ports, can you please take care of the mips64 and ppc64 implementation of the second output? BUG= TEST=cctest/test-run-machops/RunTryTruncateFloat64ToInt64WithCheck ========== to ========== MIPS64: [turbofan] Changed TruncateFloat64ToInt64 to TryTruncateFloat64ToInt64 Port 95844d94f3336489403c7f2d70c6ea01a0cf3002 Original commit message: The new operator provides a second output which indicates whether the conversion from float64 to int64 was successful or not. The second output returns 0 if the conversion fails. If the conversion succeeds, then the second output is differs from 0. The second output can be ignored, which means that the operator can be used the same way as the original operator. I implemented the new operator on x64 and arm64. @v8-mips-ports and @v8-ppc-ports, can you please take care of the mips64 and ppc64 implementation of the second output? BUG= TEST=cctest/test-run-machops/RunTryTruncateFloat64ToInt64WithCheck Committed: https://crrev.com/80f2a6390cca5fe4b2b2f97cf106b61126d9f741 Cr-Commit-Position: refs/heads/master@{#32664} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/80f2a6390cca5fe4b2b2f97cf106b61126d9f741 Cr-Commit-Position: refs/heads/master@{#32664}
Message was sent while issue was closed.
https://codereview.chromium.org/1503173002/diff/1/src/compiler/mips64/instruc... File src/compiler/mips64/instruction-selector-mips64.cc (right): https://codereview.chromium.org/1503173002/diff/1/src/compiler/mips64/instruc... src/compiler/mips64/instruction-selector-mips64.cc:1498: switch (node->opcode()) { Should kTryTruncateFloat64ToInt64 be handled in this switch?
Message was sent while issue was closed.
On 2015/12/08 14:27:42, ahaas wrote: > https://codereview.chromium.org/1503173002/diff/1/src/compiler/mips64/instruc... > File src/compiler/mips64/instruction-selector-mips64.cc (right): > > https://codereview.chromium.org/1503173002/diff/1/src/compiler/mips64/instruc... > src/compiler/mips64/instruction-selector-mips64.cc:1498: switch (node->opcode()) > { > Should kTryTruncateFloat64ToInt64 be handled in this switch? We follow the arm64 port, but this change is not included in the arm64 port, however it's in the x64. Is it required to be different on arm64 and x64 or it's just a typo? If it's intentionally different, could you please explain why?
Message was sent while issue was closed.
On 2015/12/08 at 21:04:19, akos.palfi wrote: > On 2015/12/08 14:27:42, ahaas wrote: > > https://codereview.chromium.org/1503173002/diff/1/src/compiler/mips64/instruc... > > File src/compiler/mips64/instruction-selector-mips64.cc (right): > > > > https://codereview.chromium.org/1503173002/diff/1/src/compiler/mips64/instruc... > > src/compiler/mips64/instruction-selector-mips64.cc:1498: switch (node->opcode()) > > { > > Should kTryTruncateFloat64ToInt64 be handled in this switch? > > We follow the arm64 port, but this change is not included in the arm64 port, however it's in the x64. Is it required to be different on arm64 and x64 or it's just a typo? If it's intentionally different, could you please explain why? It was just a typo. The code is part of an optimization which I misunderstood. I will remove the code from the x64 implementation. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
