Thank you for addressing the changes from yesterday. I think it's starting
to
look good. Also the assembler tests are fine.
With the following patch
Index: test/cctest/cctest.status
===================================================================
--- test/cctest/cctest.status (revision 3220)
+++ test/cctest/cctest.status (working copy)
@@ -73,3 +73,23 @@
# the JavaScript stacks are separate.
test-api/ExceptionOrder: FAIL
test-api/TryCatchInTryFinally: FAIL
+
+[ $arch == mips ]
+test-accessors: SKIP
+test-alloc: SKIP
+test-api: SKIP
+test-compiler: SKIP
+test-debug: SKIP
+test-decls: SKIP
+test-func-name-inference: SKIP
+test-heap: SKIP
+test-heap-profiler: SKIP
+test-log: SKIP
+test-log-utils: SKIP
+test-mark-compact: SKIP
+test-regexp: SKIP
+test-serialize: SKIP
+test-sockets: SKIP
+test-strings: SKIP
+test-threads: SKIP
+test-thread-termination: SKIP
I can now run
tools/test.py --simulator=mips cctest
and get
47 parsing tests (5 of those being the MIPS assembler tests).
Here are some more comments.
Also please look into removing the changes to src/jump-target.* - they
should
not be required to get the assembler,
disassembler and simulator in.
As CodeGen is not doing anything right now please trim it even further so
that
it just compiles. I think it should be possible to get rid of the
virtual-frame*
and register-allocator* for just getting the assembler, disassembler and
simulator in.
Thanks,
Søren
http://codereview.chromium.org/543161/diff/8002/9052
File SConstruct (right):
http://codereview.chromium.org/543161/diff/8002/9052#newcode849
SConstruct:849: options['profilingsupport'] = 'on'
If you add the following here you don't need to pass regexp=interpreted
when running SCons.
if options['arch'] == 'mips':
if ('regexp' in ARGUMENTS) and options['regexp'] == 'native':
# Print a warning if native regexp is specified for mips
print "Warning: forcing regexp to interpreted for mips"
options['regexp'] = 'interpreted'
http://codereview.chromium.org/543161/diff/8002/9017
File src/code-stubs.h (right):
http://codereview.chromium.org/543161/diff/8002/9017#newcode63
src/code-stubs.h:63: #define CODE_STUB_LIST_ARM(V)
Please revert this file, as these changes are not required to get the
assembler, disassembler and simulator in.
http://codereview.chromium.org/543161/diff/8002/9006
File src/codegen.h (right):
http://codereview.chromium.org/543161/diff/8002/9006#newcode151
src/codegen.h:151: #ifndef V8_TARGET_ARCH_MIPS
Please revert this change, as it is not required to get the assembler,
disassembler and simulator in.
http://codereview.chromium.org/543161/diff/8002/9006#newcode156
src/codegen.h:156: #endif
Please revert this change, as it is not required to get the assembler,
disassembler and simulator in.
Remove the function definition in codegen-mips-inl.h as well.
http://codereview.chromium.org/543161/diff/8002/9018
File src/heap.h (right):
http://codereview.chromium.org/543161/diff/8002/9018#newcode157
src/heap.h:157: #if V8_TARGET_ARCH_MIPS
Please revert this file, as it is not required to get the assembler,
disassembler and simulator in.
http://codereview.chromium.org/543161/diff/8002/9022
File src/mips/assembler-mips-inl.h (right):
http://codereview.chromium.org/543161/diff/8002/9022#newcode201
src/mips/assembler-mips-inl.h:201: return ((Assembler::instr_at(pc_) &
kOpcodeMask) == SPECIAL) &&
Please add an extra set of () around the && expression, like this (x &&
y) || z. I know the operator precedence will take care of that, but the
extra ()'s makes it more readable, and for multiline expressions it will
provide indention for redability. Also remove the outer ()'s as thay are
not needed for the return statement.
http://codereview.chromium.org/543161/diff/8002/9042
File src/mips/assembler-mips.cc (right):
http://codereview.chromium.org/543161/diff/8002/9042#newcode499
src/mips/assembler-mips.cc:499: int16_t j) {
Two spaces before j. Also 4 more times below.
http://codereview.chromium.org/543161/diff/8002/9042#newcode602
src/mips/assembler-mips.cc:602: ASSERT(is_int16(offset)); // We check
the offset can be used in a branch.
Maybe the ASSERT here and in all the other instruction generation
calling GenInstrImmediate, GenInstrRegister and GenInstrImm26 can be
moved to GenInstrImmediate, GenInstrRegister and GenInstrImm26. They all
seem to do the same for each instruction encoding format.
http://codereview.chromium.org/543161/diff/8002/9020
File src/mips/codegen-mips-inl.h (right):
http://codereview.chromium.org/543161/diff/8002/9020#newcode69
src/mips/codegen-mips-inl.h:69: void DeferredCode::Branch(Condition
cond, Register src1, const Operand& src2) {
Remove this function when codegen.h is reverted.
http://codereview.chromium.org/543161/diff/8002/9044
File src/mips/constants-mips.h (right):
http://codereview.chromium.org/543161/diff/8002/9044#newcode84
src/mips/constants-mips.h:84: static int32_t max_val() { return
kMaxValue_; }
Do you need max_val and min_val? How about just using kMaxInt and
kMinInt from globals.h. If you would like to have them here I suggest
defining
static const int32_t kMaxValue = 0x7fffffff;
static const int32_t kMinValue = 0x80000000;
public and use Registers::kMaxValue and Registers::kMinValue to access
them.
http://codereview.chromium.org/543161/diff/8002/9041
File src/mips/macro-assembler-mips.cc (right):
http://codereview.chromium.org/543161/diff/8002/9041#newcode337
src/mips/macro-assembler-mips.cc:337: lui(rd, (HIMask & j.imm32_)>>16);
Spaces around >>.
http://codereview.chromium.org/543161/diff/8002/9041#newcode339
src/mips/macro-assembler-mips.cc:339: lui(rd, (HIMask & j.imm32_)>>16);
Ditto.
http://codereview.chromium.org/543161/diff/8002/9041#newcode363
src/mips/macro-assembler-mips.cc:363: } else if (gen2instr) {
Here you are preparing the instruction stream for patching the immediate
value. However, no relocation info is recorded, so how do we end up
finding this code again to patch it?
http://codereview.chromium.org/543161/diff/8002/9041#newcode367
src/mips/macro-assembler-mips.cc:367: if (is_int16(j.imm32_)) {
If we need this anyway, then please refactor this code as it is the same
as in the "else if" above.
http://codereview.chromium.org/543161/diff/8002/9041#newcode462
src/mips/macro-assembler-mips.cc:462: case cc_always:
Do you want the li(r2, rt) abowe executed in case of cc_always?
The same goes for the other branch functions below.
http://codereview.chromium.org/543161/diff/8002/9041#newcode541
src/mips/macro-assembler-mips.cc:541: bne(scratch, zero_reg,
branch_offset(L, false)>>2);
Consider creating a function calculating "branch_offset(L, false) >> 2".
http://codereview.chromium.org/543161/diff/8002/9030
File src/mips/macro-assembler-mips.h (right):
http://codereview.chromium.org/543161/diff/8002/9030#newcode40
src/mips/macro-assembler-mips.h:40: // Register at is used for
insruction generation. So it is not safe to use it
insruction -> instruction
http://codereview.chromium.org/543161/diff/8002/9030#newcode109
src/mips/macro-assembler-mips.h:109: void jmp(Label* L) {
jmp -> Jump
http://codereview.chromium.org/543161/diff/8002/9030#newcode132
src/mips/macro-assembler-mips.h:132: #define
DEFINE_INSTRUCTION_MACRO_(instr) \
Consider removing _MACRO_.
http://codereview.chromium.org/543161/diff/8002/9030#newcode141
src/mips/macro-assembler-mips.h:141: #define
DEFINE_INSTRUCTION_MACRO_2(instr) \
Ditto.
http://codereview.chromium.org/543161/diff/8002/9030#newcode166
src/mips/macro-assembler-mips.h:166: #undef DEFINE_INSTRUCTION_MACRO_
Also undef DEFINE_INSTRUCTION_MACRO_2
http://codereview.chromium.org/543161/diff/8002/9030#newcode171
src/mips/macro-assembler-mips.h:171: void mov(Register rd, Register rt)
{ or_(rd, rt, zero_reg); }
The macros in the macro assembler are usually named XxxYxx to
distinguish them from real instructions. e.g.
push -> Push
multi_push -> PushMultiple
However commonly used pseudo-instructions (like li) should probably keep
their most common name, your call.
http://codereview.chromium.org/543161/diff/8002/9049
File src/mips/register-allocator-mips-inl.h (right):
http://codereview.chromium.org/543161/diff/8002/9049#newcode28
src/mips/register-allocator-mips-inl.h:28: #ifndef
V8_IA32_REGISTER_ALLOCATOR_IA32_INL_H_
IA32 -> MIPS (times 3=
http://codereview.chromium.org/543161
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
