Thanks for this CL, and for adding all these tests, which have been missing
for
so long!
This is only a very preliminary first review, and I only looked at
test-assembler-mips.cc in any detail. More review to come.
There are many arch-variants and modes covered by these tests. It would be
most
helpful for reviewers to know what platforms you have tested on. We really
should ensure that sim, and hardware (or qemu) have been tested in
mips32r1, r2,
r6, mips64r2, r6, and on hardware with/without FPU. That is a _lot_ of
testing
combinations.
https://codereview.chromium.org/1119203003/diff/1/test/cctest/test-assembler-mips.cc
File test/cctest/test-assembler-mips.cc (right):
https://codereview.chromium.org/1119203003/diff/1/test/cctest/test-assembler-mips.cc#newcode1332
test/cctest/test-assembler-mips.cc:1332: TEST(MIPS16) {
I've not been a fan of these numeric test names. Initially this was
taken from the early test-assembler-arm.cc. But they have gone to naming
the tests for specific functionality.
This one might be TEST(sel) or TEST(selxxx) for examples. I'm open to
your suggestion for better names, but I would like to see some names
here. Earlier tests in this file also had a comment summary right below
the test name to show what was being tested. If a single test name is
not sufficient to convey the summary, please add comment as needed.
https://codereview.chromium.org/1119203003/diff/1/test/cctest/test-assembler-mips.cc#newcode1333
test/cctest/test-assembler-mips.cc:1333: if
(IsMipsArchVariant(kMips32r6)) {
I see 3 basic categories of tests, generic for all mips32 arch variants,
mips32r2 specific, and mips32r6 specific. I think you should Order the
tests to keep all tests of a category (e.g., mips32r6) together in the
file. Maybe with some sort of "banner comment":
// ----- mips32r6 specific tests
----------------------------------------
TEST(xxxx) {
...
Same for test-assembler-mips64.
https://codereview.chromium.org/1119203003/diff/1/test/cctest/test-assembler-mips.cc#newcode1370
test/cctest/test-assembler-mips.cc:1370: __ seleqz(D, f4, f0, f2);
The seleqz functions should have assembler aliases seleqz_s and seleqz_d
(can be implemented all in assembler-mips.h, I think), and those aliases
should be used here.
https://codereview.chromium.org/1119203003/diff/1/test/cctest/test-assembler-mips.cc#newcode2821
test/cctest/test-assembler-mips.cc:2821: __ floor_l_s(f10, f6);
As we just learned in gh issue 115 we cannot use the 'long' variant of
these instructions when running on mips32 in fp32 mode. They are only
'legal' when run in fp64 mode. I believe that fp64 is selected by
default when using mips32r6, but it is definitely not default in
mips32r2.
https://codereview.chromium.org/1119203003/diff/1/test/cctest/test-assembler-mips.cc#newcode2937
test/cctest/test-assembler-mips.cc:2937: __ ceil_l_s(f10, f6);
Same as above floor, re: long variants. Maybe in other places as well.
https://codereview.chromium.org/1119203003/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
