Hi,
Thanks for addressing the comments. The assembler is looking quite nice by
now.
Some more comments below.
As Anton mentioned you updated some of the files to head. As far as I can
see
the following files are at r3737:
src/register-allocator.h (revision 3737)
src/codegen-inl.h (revision 3737)
src/flag-definitions.h (revision 3737)
src/codegen.h (revision 3737)
src/virtual-frame.h (revision 3737)
src/objects.h (revision 3737)
Whereas the rest is still at 3220:
src/SConscript (revision 3220)
src/macro-assembler.h (revision 3220)
src/assembler.h (revision 3220)
src/v8.cc (revision 3220)
src/platform-linux.cc (revision 3220)
src/frames-inl.h (revision 3220)
src/objects-inl.h (revision 3220)
src/register-allocator-inl.h (revision 3220)
src/execution.cc (revision 3220)
src/globals.h (revision 3220)
SConstruct (revision 3220)
test/cctest/test-regexp.cc (revision 3220)
test/cctest/SConscript (revision 3220)
It looks as if you have been hit by a failure in Rietveld (the codereview
tool).
It suffers from internal errors from time to time. If you click on the diff
from
patch set 4 to 5 on e.g. src/codegen.h you will get the message "Upload in
progress.", which is an indication that your "gcl upload ..." did not
complete.
I think this will be a good point for you to update your workspace to head
(you
probably already did that) and start a new code review cycle.
I have sent you a further trimmed version of src/mips/codegen* on email,
that I
suggest you use in order to have as little as possible in these files.
Regards,
Søren
http://codereview.chromium.org/543161/diff/3003/12018
File src/mips/assembler-mips-inl.h (right):
http://codereview.chromium.org/543161/diff/3003/12018#newcode203
src/mips/assembler-mips-inl.h:203: ((Assembler::instr_at(pc_) &
kFunctionFieldMask) == JALR));
You need a one character indent of the last line of the expression.
http://codereview.chromium.org/543161/diff/3003/12038
File src/mips/assembler-mips.cc (right):
http://codereview.chromium.org/543161/diff/3003/12038#newcode998
src/mips/assembler-mips.cc:998: cc << 8 | 3 << 4 | cond;
4 character indent here. Perhaps if splitting just after the = the rest
can be on one line.
http://codereview.chromium.org/543161/diff/3003/12040
File src/mips/constants-mips.h (right):
http://codereview.chromium.org/543161/diff/3003/12040#newcode35
src/mips/constants-mips.h:35: #define UNIMPLEMENTED_()
\
I suggest naming this macro UNIMPLEMENTED_MIPS
http://codereview.chromium.org/543161/diff/3003/12040#newcode38
src/mips/constants-mips.h:38: #define UNSUPPORTED_()
v8::internal::PrintF("Unsupported instruction.\n")
and UNSUPPORTED_MIPS
http://codereview.chromium.org/543161/diff/3003/12040#newcode499
src/mips/constants-mips.h:499: // Instructions are read of out a code
stream. The only way to get a
Rename isXXX -> IsXXX
http://codereview.chromium.org/543161/diff/3003/12028
File src/mips/disasm-mips.cc (right):
http://codereview.chromium.org/543161/diff/3003/12028#newcode125
src/mips/disasm-mips.cc:125: // Each of these functions decodes one
particular instruction type.
How about using names instead of numbers?
http://codereview.chromium.org/543161/diff/3003/12028#newcode164
src/mips/disasm-mips.cc:164:
Two empty lines between methods.
http://codereview.chromium.org/543161/diff/3003/12028#newcode235
src/mips/disasm-mips.cc:235: out_buffer_pos_ +=
v8i::OS::SNPrintF(out_buffer_ + out_buffer_pos_,
Either aling with ( or use 4 space indent. If splitting after += the
rest might fit on one line.
http://codereview.chromium.org/543161/diff/3003/12028#newcode246
src/mips/disasm-mips.cc:246: out_buffer_pos_ +=
v8i::OS::SNPrintF(out_buffer_ + out_buffer_pos_,
Ditto.
http://codereview.chromium.org/543161/diff/3003/12019
File src/mips/jump-target-mips.cc (right):
http://codereview.chromium.org/543161/diff/3003/12019#newcode50
src/mips/jump-target-mips.cc:50: void JumpTarget::DoBranch(Condition cc,
Hint ignored,
Remove the two Register parameters for code to compile.
http://codereview.chromium.org/543161/diff/3003/12026
File src/mips/macro-assembler-mips.h (right):
http://codereview.chromium.org/543161/diff/3003/12026#newcode256
src/mips/macro-assembler-mips.h:256: //
---------------------------------------------------------------------------
Please remove all the which are specific to V8 code generation and add
them on a needed basis.
http://codereview.chromium.org/543161/diff/3003/12026#newcode490
src/mips/macro-assembler-mips.h:490: class CodePatcher {
Just remove the code patcher for now and add it back when needed.
http://codereview.chromium.org/543161/diff/3003/12035
File src/mips/regexp-macro-assembler-mips.cc (right):
http://codereview.chromium.org/543161/diff/3003/12035#newcode1
src/mips/regexp-macro-assembler-mips.cc:1: // Copyright 2006-2008 the V8
project authors. All rights reserved.
Please remove this file as it is no longer needed.
http://codereview.chromium.org/543161/diff/3003/12022
File src/mips/regexp-macro-assembler-mips.h (right):
http://codereview.chromium.org/543161/diff/3003/12022#newcode1
src/mips/regexp-macro-assembler-mips.h:1: // Copyright 2006-2008 the V8
project authors. All rights reserved.
Please remove this file as it is no longer needed.
http://codereview.chromium.org/543161/diff/3003/12041
File src/mips/simulator-mips.cc (right):
http://codereview.chromium.org/543161/diff/3003/12041#newcode1231
src/mips/simulator-mips.cc:1231: // case TGEI:
Code in comment.
http://codereview.chromium.org/543161/diff/3003/12041#newcode1252
src/mips/simulator-mips.cc:1252: // case TGEI:
Code in comment.
http://codereview.chromium.org/543161/diff/3003/12044
File src/mips/simulator-mips.h (right):
http://codereview.chromium.org/543161/diff/3003/12044#newcode208
src/mips/simulator-mips.h:208: // Executing is handled based on the
instruction type.
How about using names instead of numbers here?
http://codereview.chromium.org/543161/diff/3003/12001
File test/cctest/test-assembler-mips.cc (right):
http://codereview.chromium.org/543161/diff/3003/12001#newcode180
test/cctest/test-assembler-mips.cc:180: // __ mfhi(v1);
Code in comments, and below.
http://codereview.chromium.org/543161/diff/3003/12049
File test/cctest/test-regexp.cc (right):
http://codereview.chromium.org/543161/diff/3003/12049#newcode1
test/cctest/test-regexp.cc:1: // Copyright 2008 the V8 project authors.
All rights reserved.
You can revert this file when src/mips/regexp-macro-assembler-mips.h is
gone.
http://codereview.chromium.org/543161
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
