Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1747863002:
MIPS: Tests for convert and truncate instructions. (Closed)
|
Created:
4 years, 9 months ago by Ilija.Pavlovic1 Modified:
4 years, 9 months ago Reviewers:
ivica.bogosavljevic, Ilija.Pavlovic1, balazs.kilvady, Marija Antic, 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: Tests for convert and truncate instructions.
Implementation new test cases for conversion instructions Cvt_s_uw,
Cvt_s_ul, Cvt_d_ul and truncate instructions Trunc_uw_s, Trunc_ul_s,
Trunc_ul_d, Trunc_l_d, Trunc_l_ud, Trunc_w_d.
TEST=cctest/test-macro-assembler-mips/cvt_s_w_Trunc_uw_s, others
cctest/test-macro-assembler-mips64/Cvt_s_uw_Trunc_uw_s, others
BUG=
Committed: https://crrev.com/b29846c283e7fed774d633bb78bb3677d0b45fa8
Cr-Commit-Position: refs/heads/master@{#34618}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Update according comments. #
Total comments: 2
Patch Set 3 : Removed unused protorypes F2, F3, F4. #
Total comments: 8
Patch Set 4 : Corrections and added tests for truncate instructions. #Messages
Total messages: 21 (7 generated)
PTAL
Looks good. Added my first round comments. https://codereview.chromium.org/1747863002/diff/1/test/cctest/test-macro-asse... File test/cctest/test-macro-assembler-mips64.cc (right): https://codereview.chromium.org/1747863002/diff/1/test/cctest/test-macro-asse... test/cctest/test-macro-assembler-mips64.cc:559: struct TestCase tc[] = { Rename this const array to uint32_test_values and move outside of the function. Instead of using a struct to store each exp/value pair, use a simple uint32_t array and do the casting inside the for {} block. https://codereview.chromium.org/1747863002/diff/1/test/cctest/test-macro-asse... test/cctest/test-macro-assembler-mips64.cc:606: struct TestCase tc[] = { Same here, call it uint64_test_values. https://codereview.chromium.org/1747863002/diff/1/test/cctest/test-macro-asse... test/cctest/test-macro-assembler-mips64.cc:653: struct TestCase tc[] = { Same here.
https://codereview.chromium.org/1747863002/diff/1/test/cctest/test-macro-asse... File test/cctest/test-macro-assembler-mips64.cc (right): https://codereview.chromium.org/1747863002/diff/1/test/cctest/test-macro-asse... test/cctest/test-macro-assembler-mips64.cc:559: struct TestCase tc[] = { On 2016/02/29 12:28:22, akos.palfi.imgtec wrote: > Rename this const array to uint32_test_values and move outside of the function. > Instead of using a struct to store each exp/value pair, use a simple uint32_t > array and do the casting inside the for {} block. In all our test suites (test-assembler-mips*, test-macro-assebler-mips*) following structure is used in most cases: TEST(<test_name>) { <test structures> <test data> <test execution> <checking received results> } If I change the code according the comment, we will have test data outside the test and we'll have a new style: <test data> TEST(<test_name>) { ... } Instead this change, I would like to introduce a macro above the TEST() similar as in following example: #define CVT_S_UW_SAMPLE(x) {x, static_cast<float>(x)} On this way, we will avoid entering duplicated data and still follow existing style. What is your opinion about this?
https://codereview.chromium.org/1747863002/diff/1/test/cctest/test-macro-asse... File test/cctest/test-macro-assembler-mips64.cc (right): https://codereview.chromium.org/1747863002/diff/1/test/cctest/test-macro-asse... test/cctest/test-macro-assembler-mips64.cc:559: struct TestCase tc[] = { On 2016/02/29 15:00:27, Ilija.Pavlovic1 wrote: > On 2016/02/29 12:28:22, akos.palfi.imgtec wrote: > > Rename this const array to uint32_test_values and move outside of the > function. > > Instead of using a struct to store each exp/value pair, use a simple uint32_t > > array and do the casting inside the for {} block. > > In all our test suites (test-assembler-mips*, test-macro-assebler-mips*) > following structure is used in most cases: > > TEST(<test_name>) { > <test structures> > <test data> > <test execution> > <checking received results> > } > > If I change the code according the comment, we will have test data outside the > test and we'll have a new style: > > <test data> > TEST(<test_name>) { > ... > } > > Instead this change, I would like to introduce a macro above the TEST() similar > as in following example: > #define CVT_S_UW_SAMPLE(x) {x, static_cast<float>(x)} > > On this way, we will avoid entering duplicated data and still follow existing > style. What is your opinion about this? Done. https://codereview.chromium.org/1747863002/diff/1/test/cctest/test-macro-asse... test/cctest/test-macro-assembler-mips64.cc:606: struct TestCase tc[] = { On 2016/02/29 12:28:22, akos.palfi.imgtec wrote: > Same here, call it uint64_test_values. Done. https://codereview.chromium.org/1747863002/diff/1/test/cctest/test-macro-asse... test/cctest/test-macro-assembler-mips64.cc:653: struct TestCase tc[] = { On 2016/02/29 12:28:22, akos.palfi.imgtec wrote: > Same here. Done.
Nit in the description: s/test--macro-assembler-mips64/test-macro-assembler-mips64/ https://codereview.chromium.org/1747863002/diff/20001/test/cctest/test-macro-... File test/cctest/test-macro-assembler-mips64.cc (right): https://codereview.chromium.org/1747863002/diff/20001/test/cctest/test-macro-... test/cctest/test-macro-assembler-mips64.cc:44: typedef float (*F2)(uint32_t x0, int x1, int x2, int x3, int x4); Nit: These defs are not necessary anymore.
Description was changed from ========== MIPS: Add test cases for conversion instructions. New test cases are implemented for conversion instructions Cvt_s_uw, Cvt_s_ul and Cvt_d_ul. TEST=cctest/test--macro-assembler-mips64/Cvt_s_uw, Cvt_s_ul, Cvt_d_ul BUG= ========== to ========== MIPS64: Add test cases for conversion instructions. New test cases are implemented for conversion instructions Cvt_s_uw, Cvt_s_ul and Cvt_d_ul. TEST=cctest/test--macro-assembler-mips64/Cvt_s_uw, Cvt_s_ul, Cvt_d_ul BUG= ==========
Description was changed from ========== MIPS64: Add test cases for conversion instructions. New test cases are implemented for conversion instructions Cvt_s_uw, Cvt_s_ul and Cvt_d_ul. TEST=cctest/test--macro-assembler-mips64/Cvt_s_uw, Cvt_s_ul, Cvt_d_ul BUG= ========== to ========== MIPS64: Add test cases for conversion instructions. New test cases are implemented for conversion instructions Cvt_s_uw, Cvt_s_ul and Cvt_d_ul. TEST=cctest/test-macro-assembler-mips64/Cvt_s_uw, Cvt_s_ul, Cvt_d_ul BUG= ==========
Removed unused prototypes F2, F3, F4. Corrected description. https://codereview.chromium.org/1747863002/diff/20001/test/cctest/test-macro-... File test/cctest/test-macro-assembler-mips64.cc (right): https://codereview.chromium.org/1747863002/diff/20001/test/cctest/test-macro-... test/cctest/test-macro-assembler-mips64.cc:44: typedef float (*F2)(uint32_t x0, int x1, int x2, int x3, int x4); On 2016/03/04 09:02:07, akos.palfi.imgtec wrote: > Nit: These defs are not necessary anymore. Done.
https://codereview.chromium.org/1747863002/diff/40001/test/cctest/test-macro-... File test/cctest/test-macro-assembler-mips64.cc (right): https://codereview.chromium.org/1747863002/diff/40001/test/cctest/test-macro-... test/cctest/test-macro-assembler-mips64.cc:525: static const std::vector<uint32_t> uint32_test_values() { should be renamed to cvt_test_values or similar, since these test vectors are used for cvt instructions https://codereview.chromium.org/1747863002/diff/40001/test/cctest/test-macro-... test/cctest/test-macro-assembler-mips64.cc:532: static const std::vector<uint64_t> uint64_test_values() { Same as above https://codereview.chromium.org/1747863002/diff/40001/test/cctest/test-macro-... test/cctest/test-macro-assembler-mips64.cc:550: RET_TYPE run_Cvt(IN_TYPE x, Func GenerateConvertInstructionFunc) { Very interesting. Leaner code, although more difficult to understand. This looks good from the point of what is being tested.
Build v8 for simulator with this patch shows unexpected problems and investigation about it is ongoing. https://codereview.chromium.org/1747863002/diff/40001/test/cctest/test-macro-... File test/cctest/test-macro-assembler-mips64.cc (right): https://codereview.chromium.org/1747863002/diff/40001/test/cctest/test-macro-... test/cctest/test-macro-assembler-mips64.cc:525: static const std::vector<uint32_t> uint32_test_values() { On 2016/03/04 16:28:28, ivica.bogosavljevic wrote: > should be renamed to cvt_test_values or similar, since these test vectors are > used for cvt instructions The model for this form of input values is similar with input values defined in the file "value-helper.h". This name "uint32_test_values" is left intentionally. The main aim is to have one set of input values which cane be used in another tests also - not just in these "cvt" tests. https://codereview.chromium.org/1747863002/diff/40001/test/cctest/test-macro-... test/cctest/test-macro-assembler-mips64.cc:525: static const std::vector<uint32_t> uint32_test_values() { On 2016/03/04 16:28:28, ivica.bogosavljevic wrote: > should be renamed to cvt_test_values or similar, since these test vectors are > used for cvt instructions The model for this form of input values is similar with input values defined in the file "value-helper.h". This name "uint32_test_values" is left intentionally. The main aim is to have one set of input values which cane be used in another tests also - not just in these "cvt" tests. https://codereview.chromium.org/1747863002/diff/40001/test/cctest/test-macro-... test/cctest/test-macro-assembler-mips64.cc:525: static const std::vector<uint32_t> uint32_test_values() { On 2016/03/04 16:28:28, ivica.bogosavljevic wrote: > should be renamed to cvt_test_values or similar, since these test vectors are > used for cvt instructions Acknowledged. https://codereview.chromium.org/1747863002/diff/40001/test/cctest/test-macro-... test/cctest/test-macro-assembler-mips64.cc:532: static const std::vector<uint64_t> uint64_test_values() { On 2016/03/04 16:28:28, ivica.bogosavljevic wrote: > Same as above Acknowledged. See previous comment. https://codereview.chromium.org/1747863002/diff/40001/test/cctest/test-macro-... test/cctest/test-macro-assembler-mips64.cc:550: RET_TYPE run_Cvt(IN_TYPE x, Func GenerateConvertInstructionFunc) { On 2016/03/04 16:28:28, ivica.bogosavljevic wrote: > Very interesting. Leaner code, although more difficult to understand. This looks > good from the point of what is being tested. Acknowledged.
PTAL. This patch has: - Adaptations Cvt tests according simulator possibilities. - New test cases for truncate instructions.
Description was changed from
==========
MIPS64: Add test cases for conversion instructions.
New test cases are implemented for conversion instructions Cvt_s_uw,
Cvt_s_ul and Cvt_d_ul.
TEST=cctest/test-macro-assembler-mips64/Cvt_s_uw, Cvt_s_ul, Cvt_d_ul
BUG=
==========
to
==========
MIPS: Tests for convert and truncate instructions.
Implementation new test cases for conversion instructions Cvt_s_uw,
Cvt_s_ul, Cvt_d_ul and truncate instructions Trunc_uw_s, Trunc_ul_s,
Trunc_ul_d, Trunc_l_d, Trunc_l_ud, Trunc_w_d.
TEST=cctest/test-macro-assembler-mips/cvt_s_w_Trunc_uw_s, others
cctest/test-macro-assembler-mips64/Cvt_s_uw_Trunc_uw_s, others
BUG=
==========
l-g-t-m, really nice. But it would be nice if Ákos takes a look also.
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/1747863002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1747863002/60001
Message was sent while issue was closed.
Description was changed from
==========
MIPS: Tests for convert and truncate instructions.
Implementation new test cases for conversion instructions Cvt_s_uw,
Cvt_s_ul, Cvt_d_ul and truncate instructions Trunc_uw_s, Trunc_ul_s,
Trunc_ul_d, Trunc_l_d, Trunc_l_ud, Trunc_w_d.
TEST=cctest/test-macro-assembler-mips/cvt_s_w_Trunc_uw_s, others
cctest/test-macro-assembler-mips64/Cvt_s_uw_Trunc_uw_s, others
BUG=
==========
to
==========
MIPS: Tests for convert and truncate instructions.
Implementation new test cases for conversion instructions Cvt_s_uw,
Cvt_s_ul, Cvt_d_ul and truncate instructions Trunc_uw_s, Trunc_ul_s,
Trunc_ul_d, Trunc_l_d, Trunc_l_ud, Trunc_w_d.
TEST=cctest/test-macro-assembler-mips/cvt_s_w_Trunc_uw_s, others
cctest/test-macro-assembler-mips64/Cvt_s_uw_Trunc_uw_s, others
BUG=
==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from
==========
MIPS: Tests for convert and truncate instructions.
Implementation new test cases for conversion instructions Cvt_s_uw,
Cvt_s_ul, Cvt_d_ul and truncate instructions Trunc_uw_s, Trunc_ul_s,
Trunc_ul_d, Trunc_l_d, Trunc_l_ud, Trunc_w_d.
TEST=cctest/test-macro-assembler-mips/cvt_s_w_Trunc_uw_s, others
cctest/test-macro-assembler-mips64/Cvt_s_uw_Trunc_uw_s, others
BUG=
==========
to
==========
MIPS: Tests for convert and truncate instructions.
Implementation new test cases for conversion instructions Cvt_s_uw,
Cvt_s_ul, Cvt_d_ul and truncate instructions Trunc_uw_s, Trunc_ul_s,
Trunc_ul_d, Trunc_l_d, Trunc_l_ud, Trunc_w_d.
TEST=cctest/test-macro-assembler-mips/cvt_s_w_Trunc_uw_s, others
cctest/test-macro-assembler-mips64/Cvt_s_uw_Trunc_uw_s, others
BUG=
Committed: https://crrev.com/b29846c283e7fed774d633bb78bb3677d0b45fa8
Cr-Commit-Position: refs/heads/master@{#34618}
==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/b29846c283e7fed774d633bb78bb3677d0b45fa8 Cr-Commit-Position: refs/heads/master@{#34618} |
