Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1453373002:
MIPS: Improve Cvt_d_uw on mips32. (Closed)
DescriptionMIPS: Improve Cvt_d_uw on mips32.
BUG=
Committed: https://crrev.com/000baddfd3ec4000df1cd4c83fcb676a8408b765
Cr-Commit-Position: refs/heads/master@{#32418}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Fixes for comments #
Total comments: 6
Patch Set 3 : addes tests, also removes redundant mfc1/mtc1 instrs. #Patch Set 4 : Further improvements #
Total comments: 2
Patch Set 5 : remove t9 #Patch Set 6 : fix compilation errors. #
Messages
Total messages: 25 (11 generated)
Description was changed from ========== Improve Cvt_d_uw on mips32. BUG= ========== to ========== Improve Cvt_d_uw on mips32. BUG= ==========
alan.li@imgtec.com changed reviewers: + akos.palfi@imgtec.com, paul.lind@imgtec.com
Issue title should be 'MIPS: Improve Cvt_d_uw. I think you should add tests for this, to be sure all the boundary cases work properly. Tests for this function must go in cctest/test-macro-assembler-mips.cc There are not too many macro-asm tests, but please see the nice tests that Djordje did in test-assembler-mips.cc, examples: TEST(rint_s), TEST(mina_maxa) For this, you want to test values at/near 0, 0x7fff.ffff, 0x8000.0000, and 0xffff.ffff Finally, as we discussed on slack, there are several callers of this function which may be non-optimal. Why don't you check those. https://codereview.chromium.org/1453373002/diff/1/src/mips/macro-assembler-mi... File src/mips/macro-assembler-mips.cc (right): https://codereview.chromium.org/1453373002/diff/1/src/mips/macro-assembler-mi... src/mips/macro-assembler-mips.cc:1274: // on FP64Mode we can simple do convertion from long Please start comments with capital letter, and end with a period. https://codereview.chromium.org/1453373002/diff/1/src/mips/macro-assembler-mi... src/mips/macro-assembler-mips.cc:1277: // Move the data from fs to t8. There is no need for this comment, it just explains what the instruction does, so it's redundant. Comments should be used when the intent might not be clear. https://codereview.chromium.org/1453373002/diff/1/src/mips/macro-assembler-mi... src/mips/macro-assembler-mips.cc:1288: // then adding 2^31 to the result (if needed). This comment block is stale, you changed the function, it no longer works this way. Please update the comment. You are doing some tricky stuff with treating an unsigned number in range 0x8000.0000 to 0xffff.ffff as a signed (negative) number, converting that to FP, then adding 2^32 to it. That is not so obvious, and should be explained by comment. https://codereview.chromium.org/1453373002/diff/1/src/mips/macro-assembler-mi... src/mips/macro-assembler-mips.cc:1295: Branch(&msb_clear, ge, rs, Operand(zero_reg)); you can use delay slot here to save an instruction. The li() below is a single instruction (lui), and if you take the branch it doesn't matter if you overwrite 'at'. If you do this, I would recommend writing it as 'lui(at, 0x41f0); // FP value: 2^32, in delay slot. https://codereview.chromium.org/1453373002/diff/1/src/mips/macro-assembler-mi... src/mips/macro-assembler-mips.cc:1297: li(at, 0x41f00000); This should have a comment, about FP value of 2^32 https://codereview.chromium.org/1453373002/diff/1/src/mips/macro-assembler-mi... src/mips/macro-assembler-mips.cc:1309: add_d(fd, fd, scratch); Nice use of delay slot!
PS2 passes mips32 and 32r6 checks. https://codereview.chromium.org/1453373002/diff/1/src/mips/macro-assembler-mi... File src/mips/macro-assembler-mips.cc (right): https://codereview.chromium.org/1453373002/diff/1/src/mips/macro-assembler-mi... src/mips/macro-assembler-mips.cc:1275: cvt_d_l(fd, fs); On FP64Mode, can we assume that fs.high() is filled with 0x0 in this case?
https://codereview.chromium.org/1453373002/diff/1/src/mips/macro-assembler-mi... File src/mips/macro-assembler-mips.cc (right): https://codereview.chromium.org/1453373002/diff/1/src/mips/macro-assembler-mi... src/mips/macro-assembler-mips.cc:1274: // on FP64Mode we can simple do convertion from long On 2015/11/18 01:49:13, paul.l... wrote: > Please start comments with capital letter, and end with a period. Done. https://codereview.chromium.org/1453373002/diff/1/src/mips/macro-assembler-mi... src/mips/macro-assembler-mips.cc:1275: cvt_d_l(fd, fs); On 2015/11/18 02:53:21, alan.li wrote: > On FP64Mode, can we assume that fs.high() is filled with 0x0 in this case? Acknowledged. https://codereview.chromium.org/1453373002/diff/1/src/mips/macro-assembler-mi... src/mips/macro-assembler-mips.cc:1277: // Move the data from fs to t8. On 2015/11/18 01:49:13, paul.l... wrote: > There is no need for this comment, it just explains what the instruction does, > so it's redundant. > Comments should be used when the intent might not be clear. Done. https://codereview.chromium.org/1453373002/diff/1/src/mips/macro-assembler-mi... src/mips/macro-assembler-mips.cc:1288: // then adding 2^31 to the result (if needed). On 2015/11/18 01:49:13, paul.l... wrote: > This comment block is stale, you changed the function, it no longer works this > way. Please update the comment. > You are doing some tricky stuff with treating an unsigned number in range > 0x8000.0000 to 0xffff.ffff as a signed (negative) number, converting that to FP, > then adding 2^32 to it. That is not so obvious, and should be explained by > comment. Done. https://codereview.chromium.org/1453373002/diff/1/src/mips/macro-assembler-mi... src/mips/macro-assembler-mips.cc:1295: Branch(&msb_clear, ge, rs, Operand(zero_reg)); On 2015/11/18 01:49:13, paul.l... wrote: > you can use delay slot here to save an instruction. The li() below is a single > instruction (lui), and if you take the branch it doesn't matter if you overwrite > 'at'. > > If you do this, I would recommend writing it as 'lui(at, 0x41f0); // FP value: > 2^32, in delay slot. > Done. https://codereview.chromium.org/1453373002/diff/1/src/mips/macro-assembler-mi... src/mips/macro-assembler-mips.cc:1297: li(at, 0x41f00000); On 2015/11/18 01:49:13, paul.l... wrote: > This should have a comment, about FP value of 2^32 Done. https://codereview.chromium.org/1453373002/diff/1/src/mips/macro-assembler-mi... src/mips/macro-assembler-mips.cc:1309: add_d(fd, fd, scratch); On 2015/11/18 01:49:13, paul.l... wrote: > Nice use of delay slot! Acknowledged.
Please do the test case & edit issue title as mentioned in previous comment. https://codereview.chromium.org/1453373002/diff/20001/src/mips/macro-assemble... File src/mips/macro-assembler-mips.cc (right): https://codereview.chromium.org/1453373002/diff/20001/src/mips/macro-assemble... src/mips/macro-assembler-mips.cc:1274: // On FP64Mode we can simple do convertion from long. Nits: fix spelling and word order in this sentence. Start with: In. Then 'do simple conversion' https://codereview.chromium.org/1453373002/diff/20001/src/mips/macro-assemble... src/mips/macro-assembler-mips.cc:1285: // Convert rs to a FP value in fd (and fd + 1). Remove the '(and fd + 1); from the comment. While that is true for fp32 mode, it is well-known, and it is not true for fp64 mode. https://codereview.chromium.org/1453373002/diff/20001/src/mips/macro-assemble... src/mips/macro-assembler-mips.cc:1298: // then add 2^32 on the result to complete the casting. Comment is too wordy, and strangely worded, we don't need 'consider this fact' or 'thus'. How about: For unsigned inputs > 2^31, we convert to double as a signed int32, then add 2^32 to move it back to unsigned value in range 2^31 .. 2^32-1.
alan.li@imgtec.com changed reviewers: + balazs.kilvady@imgtec.com, djordje.pesic@imgtec.com
I will address other similar sub-optimal cases in a new CL. https://codereview.chromium.org/1453373002/diff/20001/src/mips/macro-assemble... File src/mips/macro-assembler-mips.cc (right): https://codereview.chromium.org/1453373002/diff/20001/src/mips/macro-assemble... src/mips/macro-assembler-mips.cc:1274: // On FP64Mode we can simple do convertion from long. On 2015/11/19 01:23:38, paul.l (OOO until Nov 30) wrote: > Nits: fix spelling and word order in this sentence. Start with: In. Then 'do > simple conversion' Done. https://codereview.chromium.org/1453373002/diff/20001/src/mips/macro-assemble... src/mips/macro-assembler-mips.cc:1285: // Convert rs to a FP value in fd (and fd + 1). On 2015/11/19 01:23:38, paul.l (OOO until Nov 30) wrote: > Remove the '(and fd + 1); from the comment. While that is true for fp32 mode, it > is well-known, and it is not true for fp64 mode. Done. https://codereview.chromium.org/1453373002/diff/20001/src/mips/macro-assemble... src/mips/macro-assembler-mips.cc:1298: // then add 2^32 on the result to complete the casting. On 2015/11/19 01:23:38, paul.l (OOO until Nov 30) wrote: > Comment is too wordy, and strangely worded, we don't need 'consider this fact' > or 'thus'. How about: > > For unsigned inputs > 2^31, we convert to double as a signed int32, then add > 2^32 to move it back to unsigned value in range 2^31 .. 2^32-1. Done.
ivica.bogosavljevic@imgtec.com changed reviewers: + ivica.bogosavljevic@imgtec.com
https://codereview.chromium.org/1453373002/diff/60001/src/mips/macro-assemble... File src/mips/macro-assembler-mips.cc (right): https://codereview.chromium.org/1453373002/diff/60001/src/mips/macro-assemble... src/mips/macro-assembler-mips.cc:1279: DCHECK(!rs.is(t9)); On one hand, I wished you weren't using register t9 in macro assembler, since this ought to be available to others On the other hand, I know that t9 is used in MacroAssembler elsewhere. You added DCHECKs here, so it's ok. If you could rewrite this without using t9, that would be nice, if not it's ok like this too.
https://codereview.chromium.org/1453373002/diff/60001/src/mips/macro-assemble... File src/mips/macro-assembler-mips.cc (right): https://codereview.chromium.org/1453373002/diff/60001/src/mips/macro-assemble... src/mips/macro-assembler-mips.cc:1279: DCHECK(!rs.is(t9)); On 2015/11/23 08:56:45, ivica.bogosavljevic wrote: > On one hand, I wished you weren't using register t9 in macro assembler, since > this ought to be available to others Well, I was going to point out that t9 (and t8) are reserved for macro-assembler (https://chromium.googlesource.com/v8/v8/+/1d1557d8fded51c323e01b4ca5e36be2861...) so the usage is OK. But as I look at the code, it no longer uses t9, so Alan, just delete this DCHECK() and everyone will be happy!
So the latest patch set passes tests on simulator on both r6 and r2.
lgtm. Please change the description to 'MIPS: Improve Cvt_d_uw.'
Description was changed from ========== Improve Cvt_d_uw on mips32. BUG= ========== to ========== MIPS: Improve Cvt_d_uw on mips32. BUG= ==========
The CQ bit was checked by alan.li@imgtec.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1453373002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1453373002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
The CQ bit was checked by alan.li@imgtec.com
The patchset sent to the CQ was uploaded after l-g-t-m from balazs.kilvady@imgtec.com Link to the patchset: https://codereview.chromium.org/1453373002/#ps100001 (title: "fix compilation errors.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1453373002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1453373002/100001
Message was sent while issue was closed.
Description was changed from ========== MIPS: Improve Cvt_d_uw on mips32. BUG= ========== to ========== MIPS: Improve Cvt_d_uw on mips32. BUG= ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== MIPS: Improve Cvt_d_uw on mips32. BUG= ========== to ========== MIPS: Improve Cvt_d_uw on mips32. BUG= Committed: https://crrev.com/000baddfd3ec4000df1cd4c83fcb676a8408b765 Cr-Commit-Position: refs/heads/master@{#32418} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/000baddfd3ec4000df1cd4c83fcb676a8408b765 Cr-Commit-Position: refs/heads/master@{#32418} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
