Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1880953002:
MIPS64: Fix rotate instructions in simulator (Closed)
|
Created:
4 years, 8 months ago by Marija Antic Modified:
4 years, 8 months 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: Fix rotate instructions in simulator
Fix for drotr and drotr32 instructions in MIPS64 simulator.
BUG=
Committed: https://crrev.com/fdf19ffa5d20143176058a07be6ddcbbc68c6354
Cr-Commit-Position: refs/heads/master@{#35420}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Address comments and add tests #
Total comments: 2
Messages
Total messages: 24 (10 generated)
PTAL. Run_Wasm_I64Binops test is failing for Ror/Rol operators. It's a simulator issue, as tests are passing on the board and under qemu for r2/r6.
lgtm
The CQ bit was checked by marija.antic@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/1880953002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1880953002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-v8-committers". Note that this has nothing to do with OWNERS files.
Nice fix, please check my nits: https://codereview.chromium.org/1880953002/diff/1/src/mips64/disasm-mips64.cc File src/mips64/disasm-mips64.cc (right): https://codereview.chromium.org/1880953002/diff/1/src/mips64/disasm-mips64.cc... src/mips64/disasm-mips64.cc:1180: } Have you checked the diasm output? Are the number of spaces right in the above 2 Format() lines? It looks like arguments of drotr32 will be shifted to right a bit. https://codereview.chromium.org/1880953002/diff/1/src/mips64/simulator-mips64.cc File src/mips64/simulator-mips64.cc (right): https://codereview.chromium.org/1880953002/diff/1/src/mips64/simulator-mips64... src/mips64/simulator-mips64.cc:3437: // Sign-extend the 64-bit result. Is it realy necessary to use double conversion in the bellow line? If we need sign extension then a 'rt_u() >> sa()' would not be enough? Have you tested both? https://codereview.chromium.org/1880953002/diff/1/src/mips64/simulator-mips64... src/mips64/simulator-mips64.cc:3445: static_cast<const uint64_t>(sa()))); RotateRight64() gets its inputs by value so converting it to const is useless at here, also rt_u() is uint64_t already. I hope sa() also :) https://codereview.chromium.org/1880953002/diff/1/src/mips64/simulator-mips64... src/mips64/simulator-mips64.cc:3463: } Same conversion notes like above.
ilija.pavlovic@imgtec.com changed reviewers: + Ilija.Pavlovic@imgtec.com
Small suggestion: - if for one field are allowed value 0 and 1, the test "if (field == 0)" is not enough for assumption that the field has value 1, because the value can be also != 1. Generally, this is OK. https://codereview.chromium.org/1880953002/diff/1/src/mips64/disasm-mips64.cc File src/mips64/disasm-mips64.cc (right): https://codereview.chromium.org/1880953002/diff/1/src/mips64/disasm-mips64.cc... src/mips64/disasm-mips64.cc:1175: case DSRL32: Missing test cases in test-disassembler-mips64.cc for dsrl32 and drotr32. Also consider using construction switch(instr->RsValue()) {...} https://codereview.chromium.org/1880953002/diff/1/src/mips64/simulator-mips64.cc File src/mips64/simulator-mips64.cc (right): https://codereview.chromium.org/1880953002/diff/1/src/mips64/simulator-mips64... src/mips64/simulator-mips64.cc:3438: alu_out = static_cast<int64_t>(static_cast<uint64_t>(rt_u()) >> sa()); rt_u() already returns uint64_t. https://codereview.chromium.org/1880953002/diff/1/src/mips64/simulator-mips64... src/mips64/simulator-mips64.cc:3448: break; Rs field has defined values 0 and 1. Other values are not defined. Therefore I suggest following logic for this: case DSRL: { switch (rs_reg()) { case 0: // DSRL break; case 1: // DROTR break; default: UNREACHABLE(); } SetResult(rd_reg(), alu_out); break; } https://codereview.chromium.org/1880953002/diff/1/src/mips64/simulator-mips64... src/mips64/simulator-mips64.cc:3465: break; To use "switch-case" as for DSRL.
marija.antic@imgtec.com changed reviewers: - Ilija.Pavlovic@imgtec.com
Adding disasm tests for drotr32, and addressing comments. https://codereview.chromium.org/1880953002/diff/1/src/mips64/disasm-mips64.cc File src/mips64/disasm-mips64.cc (right): https://codereview.chromium.org/1880953002/diff/1/src/mips64/disasm-mips64.cc... src/mips64/disasm-mips64.cc:1175: case DSRL32: On 2016/04/12 12:31:31, Ilija.Pavlovic1 wrote: > Missing test cases in test-disassembler-mips64.cc for dsrl32 and drotr32. > Also consider using construction > switch(instr->RsValue()) {...} Done. https://codereview.chromium.org/1880953002/diff/1/src/mips64/disasm-mips64.cc... src/mips64/disasm-mips64.cc:1180: } On 2016/04/12 10:18:00, balazs.kilvady wrote: > Have you checked the diasm output? Are the number of spaces right in the above 2 > Format() lines? It looks like arguments of drotr32 will be shifted to right a > bit. Done. https://codereview.chromium.org/1880953002/diff/1/src/mips64/simulator-mips64.cc File src/mips64/simulator-mips64.cc (right): https://codereview.chromium.org/1880953002/diff/1/src/mips64/simulator-mips64... src/mips64/simulator-mips64.cc:3437: // Sign-extend the 64-bit result. On 2016/04/12 10:18:00, balazs.kilvady wrote: > Is it realy necessary to use double conversion in the bellow line? If we need > sign extension then a 'rt_u() >> sa()' would not be enough? Have you tested > both? Done. https://codereview.chromium.org/1880953002/diff/1/src/mips64/simulator-mips64... src/mips64/simulator-mips64.cc:3445: static_cast<const uint64_t>(sa()))); On 2016/04/12 10:18:00, balazs.kilvady wrote: > RotateRight64() gets its inputs by value so converting it to const is useless at > here, also rt_u() is uint64_t already. I hope sa() also :) Done. https://codereview.chromium.org/1880953002/diff/1/src/mips64/simulator-mips64... src/mips64/simulator-mips64.cc:3448: break; On 2016/04/12 12:31:32, Ilija.Pavlovic1 wrote: > Rs field has defined values 0 and 1. Other values are not defined. Therefore I > suggest following logic for this: > case DSRL: { > switch (rs_reg()) { > case 0: > // DSRL > break; > case 1: > // DROTR > break; > default: > UNREACHABLE(); > } > SetResult(rd_reg(), alu_out); > break; > } Done. https://codereview.chromium.org/1880953002/diff/1/src/mips64/simulator-mips64... src/mips64/simulator-mips64.cc:3465: break; Keeping it with rs_reg() (it does the same), in order to resemble the rest of simulator code. On 2016/04/12 12:31:32, Ilija.Pavlovic1 wrote: > To use "switch-case" as for DSRL.
lgtm. Could you fix the same unnecessary casting code in sim for SRL? https://codereview.chromium.org/1880953002/diff/20001/src/mips64/simulator-mi... File src/mips64/simulator-mips64.cc (right): https://codereview.chromium.org/1880953002/diff/20001/src/mips64/simulator-mi... src/mips64/simulator-mips64.cc:3422: alu_out = static_cast<int32_t>(static_cast<uint32_t>(rt_u()) >> sa()); I see. A lot of unnecessary casting existed already, you just followed it. Yep, there are quite much to clean-up. Sorry, could you fix the conversions here also? https://codereview.chromium.org/1880953002/diff/20001/src/mips64/simulator-mi... src/mips64/simulator-mips64.cc:3429: static_cast<const uint32_t>(sa()))); I see. A lot of unecessary comment existed already, you just followed it. Yep, there are quite much to clean-up. Sorry, could you fix the conversions here also?
The CQ bit was checked by marija.antic@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/1880953002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1880953002/20001
On 2016/04/12 13:14:33, balazs.kilvady wrote: > lgtm. Could you fix the same unnecessary casting code in sim for SRL? > > https://codereview.chromium.org/1880953002/diff/20001/src/mips64/simulator-mi... > File src/mips64/simulator-mips64.cc (right): > > https://codereview.chromium.org/1880953002/diff/20001/src/mips64/simulator-mi... > src/mips64/simulator-mips64.cc:3422: alu_out = > static_cast<int32_t>(static_cast<uint32_t>(rt_u()) >> sa()); > I see. A lot of unnecessary casting existed already, you just followed it. Yep, > there are quite much to clean-up. Sorry, could you fix the conversions here > also? > > https://codereview.chromium.org/1880953002/diff/20001/src/mips64/simulator-mi... > src/mips64/simulator-mips64.cc:3429: static_cast<const uint32_t>(sa()))); > I see. A lot of unecessary comment existed already, you just followed it. Yep, > there are quite much to clean-up. Sorry, could you fix the conversions here > also? Actually, these seem to be necessary. Removing the ones not required to compile the code causes test failures.
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 marija.antic@imgtec.com
The patchset sent to the CQ was uploaded after l-g-t-m from ivica.bogosavljevic@imgtec.com Link to the patchset: https://codereview.chromium.org/1880953002/#ps20001 (title: "Address comments and add tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1880953002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1880953002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== MIPS64: Fix rotate instructions in simulator Fix for drotr and drotr32 instructions in MIPS64 simulator. BUG= ========== to ========== MIPS64: Fix rotate instructions in simulator Fix for drotr and drotr32 instructions in MIPS64 simulator. BUG= Committed: https://crrev.com/fdf19ffa5d20143176058a07be6ddcbbc68c6354 Cr-Commit-Position: refs/heads/master@{#35420} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/fdf19ffa5d20143176058a07be6ddcbbc68c6354 Cr-Commit-Position: refs/heads/master@{#35420} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
