Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1867503002:
MIPS: Enable big endian testing. (Closed)
|
Created:
4 years, 8 months ago by Ilija.Pavlovic1 Modified:
4 years, 8 months ago Reviewers:
ivica.bogosavljevic, ilija.pavlovic, balazs.kilvady, miran.karic, Marija Antic, titzer, gergely.kis.imgtec, akos.palfi.imgtec 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. |
DescriptionMIPS: Enable big endian testing.
Enabled big endian testing for MIPS32 and MIPS64. The tests are also
adapted for big endian variant.
TEST=cctest/test-assembler-mips[64]
BUG=
Committed: https://crrev.com/0b479e4e81daf28494a5748dce9ed793e9d02342
Cr-Commit-Position: refs/heads/master@{#35369}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Updated according comments. #
Messages
Total messages: 21 (8 generated)
PTAL
https://codereview.chromium.org/1867503002/diff/1/test/cctest/cctest.gyp File test/cctest/cctest.gyp (right): https://codereview.chromium.org/1867503002/diff/1/test/cctest/cctest.gyp#newc... test/cctest/cctest.gyp:273: 'sources': [ ### gcmole(arch:mipsel) ### ### gcmole(arch:mips) ###. Copy-paste is not so reliable :) https://codereview.chromium.org/1867503002/diff/1/test/cctest/cctest.gyp#newc... test/cctest/cctest.gyp:291: 'sources': [ ### gcmole(arch:mips64) ###should be added behind the above [. https://codereview.chromium.org/1867503002/diff/1/test/cctest/cctest.gyp#newc... test/cctest/cctest.gyp:300: 'sources': [ ### gcmole(arch:mips64el) ###should be added behind the above [. https://codereview.chromium.org/1867503002/diff/1/test/cctest/test-assembler-... File test/cctest/test-assembler-mips64.cc (right): https://codereview.chromium.org/1867503002/diff/1/test/cctest/test-assembler-... test/cctest/test-assembler-mips64.cc:1404: T t; In C++ no need for typedef for structs, enums. Here you can use struct T {}; T t; Or even struct T {} t; I guess. https://codereview.chromium.org/1867503002/diff/1/test/cctest/test-assembler-... test/cctest/test-assembler-mips64.cc:5615: Nice solution. But if you have time, please try out the next modification: use __ dq(int64_t) in run_ldpc() (which also could be a static function). I guess in that case TestCaseLdpc::expected_res stored in the used architecture's native endianess format and ldpc() would read the double-words in the same format. I hope we could avoid order-fix tricks (although I like lambdas :) ). https://codereview.chromium.org/1867503002/diff/1/test/cctest/test-assembler-... test/cctest/test-assembler-mips64.cc:5623: struct TestCaseLdpc tc[] = { struct keyword is needless here. https://codereview.chromium.org/1867503002/diff/1/test/unittests/unittests.gyp File test/unittests/unittests.gyp (right): https://codereview.chromium.org/1867503002/diff/1/test/unittests/unittests.gy... test/unittests/unittests.gyp:143: 'sources': [ ### gcmole(arch:mipsel) ### I think it should be ### gcmole(arch:mips) ### https://codereview.chromium.org/1867503002/diff/1/test/unittests/unittests.gy... test/unittests/unittests.gyp:153: 'sources': [ ### gcmole(arch:mips64el) ### I think it should be ### gcmole(arch:mips64) ###
https://codereview.chromium.org/1867503002/diff/1/test/cctest/cctest.gyp File test/cctest/cctest.gyp (right): https://codereview.chromium.org/1867503002/diff/1/test/cctest/cctest.gyp#newc... test/cctest/cctest.gyp:273: 'sources': [ ### gcmole(arch:mipsel) ### On 2016/04/06 10:36:22, balazs.kilvady wrote: > ### gcmole(arch:mips) ###. Copy-paste is not so reliable :) Done. https://codereview.chromium.org/1867503002/diff/1/test/cctest/cctest.gyp#newc... test/cctest/cctest.gyp:291: 'sources': [ On 2016/04/06 10:36:22, balazs.kilvady wrote: > ### gcmole(arch:mips64) ###should be added behind the above [. Done. https://codereview.chromium.org/1867503002/diff/1/test/cctest/cctest.gyp#newc... test/cctest/cctest.gyp:300: 'sources': [ On 2016/04/06 10:36:22, balazs.kilvady wrote: > ### gcmole(arch:mips64el) ###should be added behind the above [. Done. https://codereview.chromium.org/1867503002/diff/1/test/cctest/test-assembler-... File test/cctest/test-assembler-mips64.cc (right): https://codereview.chromium.org/1867503002/diff/1/test/cctest/test-assembler-... test/cctest/test-assembler-mips64.cc:1404: T t; On 2016/04/06 10:36:22, balazs.kilvady wrote: > In C++ no need for typedef for structs, enums. Here you can use struct T {}; T > t; Or even struct T {} t; I guess. Done. https://codereview.chromium.org/1867503002/diff/1/test/cctest/test-assembler-... test/cctest/test-assembler-mips64.cc:5615: On 2016/04/06 10:36:22, balazs.kilvady wrote: > Nice solution. But if you have time, please try out the next modification: > use __ dq(int64_t) in run_ldpc() (which also could be a static function). I > guess in that case TestCaseLdpc::expected_res stored in the used architecture's > native endianess format and ldpc() would read the double-words in the same > format. I hope we could avoid order-fix tricks (although I like lambdas :) ). Solution with __ dq(int64_t) is tested and it works for all offset values except for offset = 0. Here is one failed example for big endian (test data are adapted for little endian): res=0xef18000003001025 expected_res=0x3001025ef180000 For offset = 0 two words are taken: - one with addiu instruction and another with ldpc instruction. Therefore for big and little endian we will have different values. https://codereview.chromium.org/1867503002/diff/1/test/cctest/test-assembler-... test/cctest/test-assembler-mips64.cc:5623: struct TestCaseLdpc tc[] = { On 2016/04/06 10:36:22, balazs.kilvady wrote: > struct keyword is needless here. Done. https://codereview.chromium.org/1867503002/diff/1/test/unittests/unittests.gyp File test/unittests/unittests.gyp (right): https://codereview.chromium.org/1867503002/diff/1/test/unittests/unittests.gy... test/unittests/unittests.gyp:143: 'sources': [ ### gcmole(arch:mipsel) ### On 2016/04/06 10:36:22, balazs.kilvady wrote: > I think it should be ### gcmole(arch:mips) ### Done. https://codereview.chromium.org/1867503002/diff/1/test/unittests/unittests.gy... test/unittests/unittests.gyp:153: 'sources': [ ### gcmole(arch:mips64el) ### On 2016/04/06 10:36:22, balazs.kilvady wrote: > I think it should be ### gcmole(arch:mips64) ### Done.
lgtm with a nit: https://codereview.chromium.org/1867503002/diff/1/test/cctest/test-assembler-... File test/cctest/test-assembler-mips64.cc (right): https://codereview.chromium.org/1867503002/diff/1/test/cctest/test-assembler-... test/cctest/test-assembler-mips64.cc:5615: On 2016/04/06 14:52:47, Ilija.Pavlovic1 wrote: > On 2016/04/06 10:36:22, balazs.kilvady wrote: > > Nice solution. But if you have time, please try out the next modification: > > use __ dq(int64_t) in run_ldpc() (which also could be a static function). I > > guess in that case TestCaseLdpc::expected_res stored in the used > architecture's > > native endianess format and ldpc() would read the double-words in the same > > format. I hope we could avoid order-fix tricks (although I like lambdas :) ). > > Solution with __ dq(int64_t) is tested and it works for all offset values except > for offset = 0. > > Here is one failed example for big endian (test data are adapted for little > endian): > > res=0xef18000003001025 expected_res=0x3001025ef180000 > > For offset = 0 two words are taken: - one with addiu instruction and another > with ldpc instruction. Therefore for big and little endian we will have > different values. Yep, as ldpc emitted alone as a 32-bit value we loose the benefit of emitting 8-byte values in arch-dependen endianess way. We could use a dq(ldpc << 32 | nop) but that is unreadble nasty hack. So lets use your first solution (till there is no better idea :) ). Also ldpc aligns pc to a 8-byte boundary: GPR[rs] memory[ (PC&~0x7) + sign_extend( offset << 3) ] which could be ensured with __ Align(8);
And please remove [64] from the description (we use MIPS64 if the CL in 64-bit only, otherwise just MIPS): MIPS: Enable big endian testing.
Description was changed from ========== MIPS[64]: Enable big endian testing. Enabled big endian testing for MIPS32 and MIPS64. The tests are also adapted for big endian variant. TEST=cctest/test-assembler-mips[64] BUG= ========== to ========== MIPS: Enable big endian testing. Enabled big endian testing for MIPS32 and MIPS64. The tests are also adapted for big endian variant. TEST=cctest/test-assembler-mips[64] BUG= ==========
https://codereview.chromium.org/1867503002/diff/1/test/cctest/test-assembler-... File test/cctest/test-assembler-mips64.cc (right): https://codereview.chromium.org/1867503002/diff/1/test/cctest/test-assembler-... test/cctest/test-assembler-mips64.cc:5615: On 2016/04/06 17:43:13, balazs.kilvady wrote: > On 2016/04/06 14:52:47, Ilija.Pavlovic1 wrote: > > On 2016/04/06 10:36:22, balazs.kilvady wrote: > > > Nice solution. But if you have time, please try out the next modification: > > > use __ dq(int64_t) in run_ldpc() (which also could be a static function). I > > > guess in that case TestCaseLdpc::expected_res stored in the used > > architecture's > > > native endianess format and ldpc() would read the double-words in the same > > > format. I hope we could avoid order-fix tricks (although I like lambdas :) > ). > > > > Solution with __ dq(int64_t) is tested and it works for all offset values > except > > for offset = 0. > > > > Here is one failed example for big endian (test data are adapted for little > > endian): > > > > res=0xef18000003001025 expected_res=0x3001025ef180000 > > > > For offset = 0 two words are taken: - one with addiu instruction and another > > with ldpc instruction. Therefore for big and little endian we will have > > different values. > > Yep, as ldpc emitted alone as a 32-bit value we loose the benefit of emitting > 8-byte values in arch-dependen endianess way. We could use a dq(ldpc << 32 | > nop) but that is unreadble nasty hack. So lets use your first solution (till > there is no better idea :) ). > > Also ldpc aligns pc to a 8-byte boundary: > GPR[rs] memory[ (PC&~0x7) + sign_extend( offset << 3) ] > which could be ensured with __ Align(8); Acknowledged.
The CQ bit was checked by ilija.pavlovic@imgtec.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1867503002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1867503002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/13348)
ilija.pavlovic@imgtec.com changed reviewers: + ilija.pavlovic@imgtec.com, titzer@chromium.org - Ilija.Pavlovic@imgtec.com
Hello, I would ask "lgtm" from titzer. Best regards Ilija
On 2016/04/07 11:15:45, Ilija.Pavlovic1 wrote: > Hello, > > I would ask "lgtm" from titzer. > > Best regards > Ilija lgtm
The CQ bit was checked by ilija.pavlovic@imgtec.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1867503002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1867503002/20001
Message was sent while issue was closed.
Description was changed from ========== MIPS: Enable big endian testing. Enabled big endian testing for MIPS32 and MIPS64. The tests are also adapted for big endian variant. TEST=cctest/test-assembler-mips[64] BUG= ========== to ========== MIPS: Enable big endian testing. Enabled big endian testing for MIPS32 and MIPS64. The tests are also adapted for big endian variant. TEST=cctest/test-assembler-mips[64] BUG= ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== MIPS: Enable big endian testing. Enabled big endian testing for MIPS32 and MIPS64. The tests are also adapted for big endian variant. TEST=cctest/test-assembler-mips[64] BUG= ========== to ========== MIPS: Enable big endian testing. Enabled big endian testing for MIPS32 and MIPS64. The tests are also adapted for big endian variant. TEST=cctest/test-assembler-mips[64] BUG= Committed: https://crrev.com/0b479e4e81daf28494a5748dce9ed793e9d02342 Cr-Commit-Position: refs/heads/master@{#35369} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/0b479e4e81daf28494a5748dce9ed793e9d02342 Cr-Commit-Position: refs/heads/master@{#35369} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
