More comments, this time in test-assembler-mips.cc. it is really nice to
see
this thorough test coverage!
I'm still not done with the review yet. Feel free to update if you are
ready.
https://codereview.chromium.org/1144373003/diff/40001/test/cctest/test-assembler-mips.cc
File test/cctest/test-assembler-mips.cc (right):
https://codereview.chromium.org/1144373003/diff/40001/test/cctest/test-assembler-mips.cc#newcode4399
test/cctest/test-assembler-mips.cc:4399: uint32_t PC; // Program
Counter
nit: period at end.
https://codereview.chromium.org/1144373003/diff/40001/test/cctest/test-assembler-mips.cc#newcode4418
test/cctest/test-assembler-mips.cc:4418: PC = (uint32_t) f; // set
program counter
nit: Start with capital, end with period. Many more in this file.
Use judgement, as the above '// ALUIPC rs, imm16' is the form of an
instruction, and would not read correctly with the trailing period. So
those kind are fine as is.
https://codereview.chromium.org/1144373003/diff/40001/test/cctest/test-assembler-mips.cc#newcode4522
test/cctest/test-assembler-mips.cc:4522: // addiu t3, a4, 0xffff;
(0x250fffff)
You have mips64 register names here. This is addiu t7, t0, 0xffff in
mips32 reg naming convention. Please adjust other nearby comments as
well.
https://codereview.chromium.org/1144373003/diff/40001/test/cctest/test-assembler-mips.cc#newcode4573
test/cctest/test-assembler-mips.cc:4573: { t8.code(), -262144,
0x250fffff }, // offset 0x40000
I do not see the point of having 'rs' parameter in the table or passing
it thru to the function. It is a register number, which you want encoded
into an instruction. But then you pass it as a parameter, which then
gets to the called function in a0 **at runtime**, you cannot assemble
the code with that. So above, you hard-code t8 into the lwpc()
instruction (and you need to do it that way -- but it makes the presence
of this value in the table not useful.)
It seems there is confusion here between
compilation/assembly-code-generation time, and runtime.
https://codereview.chromium.org/1144373003/diff/40001/test/cctest/test-assembler-mips.cc#newcode4657
test/cctest/test-assembler-mips.cc:4657: // rt - formal argument; will
contain value of the program counter
the rt parameter in this table appears unused in run_jic() , and its
presence and naming seems a bit confusing. I thought you were using that
as the 'rt' value to the JIC instruction, but on closer look I see that
is derived from PC, making this an inlined jump table of a sort. If you
are not going to use rt, I think you should remove it here, and make a
little more clear.
https://codereview.chromium.org/1144373003/diff/40001/test/cctest/test-assembler-mips.cc#newcode4660
test/cctest/test-assembler-mips.cc:4660: //{ 0, 0,
0x0 }, // JIC in loop
Don't leave in the commented out case.
https://codereview.chromium.org/1144373003/diff/40001/test/cctest/test-assembler-mips.cc#newcode4726
test/cctest/test-assembler-mips.cc:4726: uint32_t rs;
The 'rs' was confusing here, because the instruction is defined in terms
of 'rs' being a register number in the range 0-31, but you are actually
using here is the contents of register a0, which is passed in thru the
Call.
Seems like 'value' or some other name might be more descriptive.
https://codereview.chromium.org/1144373003/diff/40001/test/cctest/test-assembler-mips.cc#newcode4826
test/cctest/test-assembler-mips.cc:4826: // rt - formal argument; will
contain value of the program counter
Again, rt is unused here, I suggest removing it for clarity. Same
comment applies to other tests also (like the next one).
https://codereview.chromium.org/1144373003/diff/40001/test/cctest/test-assembler-mips.cc#newcode4913
test/cctest/test-assembler-mips.cc:4913: for (int32_t i = -100; i <=
-11; ++i) {
In my earlier test code for bc & balc I generated a LOT of instructions,
since a major point of the instruction is to have a larger branch
distance than we get with the normal branches. It might be useful to
retain that, so we make sure that the simulator, qemu, and the real
silicon all agree. This might apply to the beqzc instruction as well.
https://codereview.chromium.org/1144373003/
--
--
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.
