Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1846673003:
S390: Implemented ALCR in S390 simulator. (Closed)
DescriptionS390: Implemented ALCR in S390 simulator.
This CL implements the ALCR, add logical 32-bit integer with carry, instruction in the s390 simulator.
Some 64-bit operations in the 4-byte arithmetic section of the s390 simulator have been refactored into a separate function to stay below 500 lines.
R=michael_dawson@ca.ibm.com,jyan@ca.ibm.com,mbrandy@us.ibm.com,joransiu@ca.ibm.com,
BUG=
Committed: https://crrev.com/8760b602b702af9a9959c729833154a24abefbdc
Cr-Commit-Position: refs/heads/master@{#35184}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Removed SetS390OverflowCode from ALR and ALCR #
Total comments: 4
Patch Set 3 : Replaced check for condition_reg_ == 0 with assert that fails if condition_reg_ == 0. #
Messages
Total messages: 26 (13 generated)
Description was changed from ========== S390: Implemented ALCR in S390 simulator. Some 64-bit operations in the 4-byte arithmetic section of the s390 simulator have been refactored into a separate function to stay below 500 lines. BUG= ========== to ========== S390: Implemented ALCR in S390 simulator. This CL implements the ALCR, add logical 32-bit integer with carry, instruction in the s390 simulator. Some 64-bit operations in the 4-byte arithmetic section of the s390 simulator have been refactored into a separate function to stay below 500 lines. R=michael_dawson@ca.ibm.com,jyan@ca.ibm.com,mtbrandyberry@ca.ibm.com,joransiu... BUG= ==========
Description was changed from ========== S390: Implemented ALCR in S390 simulator. This CL implements the ALCR, add logical 32-bit integer with carry, instruction in the s390 simulator. Some 64-bit operations in the 4-byte arithmetic section of the s390 simulator have been refactored into a separate function to stay below 500 lines. R=michael_dawson@ca.ibm.com,jyan@ca.ibm.com,mtbrandyberry@ca.ibm.com,joransiu... BUG= ========== to ========== S390: Implemented ALCR in S390 simulator. This CL implements the ALCR, add logical 32-bit integer with carry, instruction in the s390 simulator. Some 64-bit operations in the 4-byte arithmetic section of the s390 simulator have been refactored into a separate function to stay below 500 lines. R=michael_dawson@ca.ibm.com,jyan@ca.ibm.com,mtbrandyberry@ca.ibm.com,joransiu..., BUG= ==========
bryleun@ca.ibm.com changed reviewers: - danno@chromium.org, jkummerow@chromium.org, jochen@chromium.org
https://codereview.chromium.org/1846673003/diff/1/src/s390/simulator-s390.cc File src/s390/simulator-s390.cc (right): https://codereview.chromium.org/1846673003/diff/1/src/s390/simulator-s390.cc#... src/s390/simulator-s390.cc:1826: SetS390OverflowCode(isOF); ALR/SLR would not set Overflow condition as stated in the poop. They only set condition code based on whether output > or = 0 and whether carry bit is produced. Could you explain why calling SetS390OverflowCode here? https://codereview.chromium.org/1846673003/diff/1/src/s390/simulator-s390.cc#... src/s390/simulator-s390.cc:3041: SetS390OverflowCode(isOF); same comment as above.
Description was changed from ========== S390: Implemented ALCR in S390 simulator. This CL implements the ALCR, add logical 32-bit integer with carry, instruction in the s390 simulator. Some 64-bit operations in the 4-byte arithmetic section of the s390 simulator have been refactored into a separate function to stay below 500 lines. R=michael_dawson@ca.ibm.com,jyan@ca.ibm.com,mtbrandyberry@ca.ibm.com,joransiu..., BUG= ========== to ========== S390: Implemented ALCR in S390 simulator. This CL implements the ALCR, add logical 32-bit integer with carry, instruction in the s390 simulator. Some 64-bit operations in the 4-byte arithmetic section of the s390 simulator have been refactored into a separate function to stay below 500 lines. R=michael_dawson@ca.ibm.com,jyan@ca.ibm.com,mbrandy@us.ibm.com,joransiu@ca.ib..., BUG= ==========
bryleun@ca.ibm.com changed reviewers: + mbrandy@us.ibm.com - mtbrandyberry@ca.ibm.com
https://codereview.chromium.org/1846673003/diff/1/src/s390/simulator-s390.cc File src/s390/simulator-s390.cc (right): https://codereview.chromium.org/1846673003/diff/1/src/s390/simulator-s390.cc#... src/s390/simulator-s390.cc:3041: SetS390OverflowCode(isOF); If isOF is true, won't this override the CC set above in the Carry?
Updated. https://codereview.chromium.org/1846673003/diff/1/src/s390/simulator-s390.cc File src/s390/simulator-s390.cc (right): https://codereview.chromium.org/1846673003/diff/1/src/s390/simulator-s390.cc#... src/s390/simulator-s390.cc:1826: SetS390OverflowCode(isOF); On 2016/03/30 20:48:52, john.yan wrote: > ALR/SLR would not set Overflow condition as stated in the poop. They only set > condition code based on whether output > or = 0 and whether carry bit is > produced. Could you explain why calling SetS390OverflowCode here? This is a leftover from ALR, which originally had the SetS390OverflowCode line present. Removing this line (and the one below) did not increase the number of failures when running no-variant tests. This line has been removed in the latest patch. https://codereview.chromium.org/1846673003/diff/1/src/s390/simulator-s390.cc#... src/s390/simulator-s390.cc:3041: SetS390OverflowCode(isOF); On 2016/03/30 20:48:52, john.yan wrote: > same comment as above. Code for ALCR was copied from AR, which had the SetS390OverflowCode(isOF) line present. As with above, removing this line didn't increase (or reduce) number of failures when running no-variant test, so this line has been removed.
lgtm
lgtm othewise. https://codereview.chromium.org/1846673003/diff/20001/src/s390/simulator-s390.h File src/s390/simulator-s390.h (right): https://codereview.chromium.org/1846673003/diff/20001/src/s390/simulator-s390... src/s390/simulator-s390.h:400: template <typename T> Not sure if this needs to be a template function, given that result is only being compared to zero. https://codereview.chromium.org/1846673003/diff/20001/src/s390/simulator-s390... src/s390/simulator-s390.h:414: // We get down here only for floating point Are there any floating point operations that would set the carry flag? If not, it might be better to stick an ASSERT to ensure condition_reg_ is non-zero.
Updated https://codereview.chromium.org/1846673003/diff/20001/src/s390/simulator-s390.h File src/s390/simulator-s390.h (right): https://codereview.chromium.org/1846673003/diff/20001/src/s390/simulator-s390... src/s390/simulator-s390.h:400: template <typename T> On 2016/03/31 13:27:57, JoranSiu wrote: > Not sure if this needs to be a template function, given that result is only > being compared to zero. This function will be used with unsigned int inputs of multiple types (e.g. uint32 and uint64), therefore it is necessary to make it a template function. https://codereview.chromium.org/1846673003/diff/20001/src/s390/simulator-s390... src/s390/simulator-s390.h:414: // We get down here only for floating point On 2016/03/31 13:27:57, JoranSiu wrote: > Are there any floating point operations that would set the carry flag? If not, > it might be better to stick an ASSERT to ensure condition_reg_ is non-zero. According to principles of operation, no floating-point operations set the carry flag. This line has been replaced with a check that causes an UNREACHABLE() error if condition_reg_ is zero.
On 2016/03/31 19:12:39, bcleung wrote: > Updated > > https://codereview.chromium.org/1846673003/diff/20001/src/s390/simulator-s390.h > File src/s390/simulator-s390.h (right): > > https://codereview.chromium.org/1846673003/diff/20001/src/s390/simulator-s390... > src/s390/simulator-s390.h:400: template <typename T> > On 2016/03/31 13:27:57, JoranSiu wrote: > > Not sure if this needs to be a template function, given that result is only > > being compared to zero. > > This function will be used with unsigned int inputs of multiple types (e.g. > uint32 and uint64), therefore it is necessary to make it a template function. > > https://codereview.chromium.org/1846673003/diff/20001/src/s390/simulator-s390... > src/s390/simulator-s390.h:414: // We get down here only for floating point > On 2016/03/31 13:27:57, JoranSiu wrote: > > Are there any floating point operations that would set the carry flag? If > not, > > it might be better to stick an ASSERT to ensure condition_reg_ is non-zero. > > According to principles of operation, no floating-point operations set the carry > flag. This line has been replaced with a check that causes an UNREACHABLE() > error if condition_reg_ is zero. lgtm
The CQ bit was checked by bryleun@ca.ibm.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/1846673003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1846673003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was unchecked by bryleun@ca.ibm.com
The CQ bit was checked by bryleun@ca.ibm.com
The patchset sent to the CQ was uploaded after l-g-t-m from jyan@ca.ibm.com Link to the patchset: https://codereview.chromium.org/1846673003/#ps40001 (title: "Replaced check for condition_reg_ == 0 with assert that fails if condition_reg_ == 0.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1846673003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1846673003/40001
Message was sent while issue was closed.
Description was changed from ========== S390: Implemented ALCR in S390 simulator. This CL implements the ALCR, add logical 32-bit integer with carry, instruction in the s390 simulator. Some 64-bit operations in the 4-byte arithmetic section of the s390 simulator have been refactored into a separate function to stay below 500 lines. R=michael_dawson@ca.ibm.com,jyan@ca.ibm.com,mbrandy@us.ibm.com,joransiu@ca.ib..., BUG= ========== to ========== S390: Implemented ALCR in S390 simulator. This CL implements the ALCR, add logical 32-bit integer with carry, instruction in the s390 simulator. Some 64-bit operations in the 4-byte arithmetic section of the s390 simulator have been refactored into a separate function to stay below 500 lines. R=michael_dawson@ca.ibm.com,jyan@ca.ibm.com,mbrandy@us.ibm.com,joransiu@ca.ib..., BUG= ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== S390: Implemented ALCR in S390 simulator. This CL implements the ALCR, add logical 32-bit integer with carry, instruction in the s390 simulator. Some 64-bit operations in the 4-byte arithmetic section of the s390 simulator have been refactored into a separate function to stay below 500 lines. R=michael_dawson@ca.ibm.com,jyan@ca.ibm.com,mbrandy@us.ibm.com,joransiu@ca.ib..., BUG= ========== to ========== S390: Implemented ALCR in S390 simulator. This CL implements the ALCR, add logical 32-bit integer with carry, instruction in the s390 simulator. Some 64-bit operations in the 4-byte arithmetic section of the s390 simulator have been refactored into a separate function to stay below 500 lines. R=michael_dawson@ca.ibm.com,jyan@ca.ibm.com,mbrandy@us.ibm.com,joransiu@ca.ib..., BUG= Committed: https://crrev.com/8760b602b702af9a9959c729833154a24abefbdc Cr-Commit-Position: refs/heads/master@{#35184} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8760b602b702af9a9959c729833154a24abefbdc Cr-Commit-Position: refs/heads/master@{#35184} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
