This is a partial review, though I suspect many of the comments in the
mips32
assembler, will carry over to mips64. I review the rest tomorrow.
https://codereview.chromium.org/1144373003/diff/40001/src/mips/assembler-mips.cc
File src/mips/assembler-mips.cc (right):
https://codereview.chromium.org/1144373003/diff/40001/src/mips/assembler-mips.cc#newcode285
src/mips/assembler-mips.cc:285: Assembler::Assembler(Isolate* isolate,
void* buffer, int buffer_size)
nit: don't delete the line here, there should be 2 blank lines before
Assembler...
https://codereview.chromium.org/1144373003/diff/40001/src/mips/assembler-mips.cc#newcode970
src/mips/assembler-mips.cc:970: DCHECK((kMinInt26 <= offset26) &&
(offset26 <= kMaxInt26));
You should be able to use is_int26() here - defined around line 1016 of
utils.h
https://codereview.chromium.org/1144373003/diff/40001/src/mips/assembler-mips.cc#newcode971
src/mips/assembler-mips.cc:971: Instr instr = opcode |
static_cast<uint32_t>(offset26 & kImm26Mask);
I don't think you need the cast here -- all the types are uint32_t
already.
https://codereview.chromium.org/1144373003/diff/40001/src/mips/assembler-mips.cc#newcode1180
src/mips/assembler-mips.cc:1180: DCHECK(IsMipsArchVariant(kMips32r6));
I suspect we will need positions_recorder()->WriteRecordedPositions();
https://codereview.chromium.org/1144373003/diff/40001/src/mips/assembler-mips.cc#newcode1385
src/mips/assembler-mips.cc:1385: static_cast<uint32_t>(offset &
kImm21Mask);
Again, I think the cast is not needed. The code will read more clearly
if we can avoid them. If you can remove them, please do below instances
as well.
https://codereview.chromium.org/1144373003/diff/40001/src/mips/assembler-mips.cc#newcode1801
src/mips/assembler-mips.cc:1801: // 'lui' has zero reg. for rs field.
You should add DCHECK(rs != zero_reg);
(I know its not your change ... but I can't help myself :)
https://codereview.chromium.org/1144373003/diff/40001/src/mips/assembler-mips.cc#newcode1812
src/mips/assembler-mips.cc:1812: DCHECK((kMinInt19 <= imm19) && (imm19
<= kMaxInt19));
I think is_int19() will do, here and below.
https://codereview.chromium.org/1144373003/diff/40001/src/mips/assembler-mips.h
File src/mips/assembler-mips.h (right):
https://codereview.chromium.org/1144373003/diff/40001/src/mips/assembler-mips.h#newcode755
src/mips/assembler-mips.h:755: // Jump targets must be in the current
256 MB-aligned region. i.e. 28 bits.
This comment goes only with the first 2 instructions here, j and jal.
Maybe add a space or a new section so that it does not seem like this
comment aapplies to jr/jalr or to your new jic/jialc.
https://codereview.chromium.org/1144373003/diff/40001/src/mips/assembler-mips.h#newcode761
src/mips/assembler-mips.h:761: void jal_or_jalr(int32_t target, Register
rs);
These 2 j_or_jr/jal_or_jalr do not appear to be used, and should be
removed from here and assembler-mips[64].cc. They cannot be used without
related patching infrastructure anyway. This doesn't pertain directly to
this CL, but they should either be removed now, or create a separate
cleanup CL for this.
https://codereview.chromium.org/1144373003/diff/40001/src/mips/constants-mips.h
File src/mips/constants-mips.h (right):
https://codereview.chromium.org/1144373003/diff/40001/src/mips/constants-mips.h#newcode248
src/mips/constants-mips.h:248: const int32_t kMinInt26 = -33554432;
I don't think you need this constants if you change assembler to use
is_intNN()
https://codereview.chromium.org/1144373003/diff/40001/src/mips/constants-mips.h#newcode267
src/mips/constants-mips.h:267: const int kBp2Bits = 2;
nit: indentation is off here and a few places below. I am guessing that
clang-format is to blame, and won't let you format like the existing
code.
Be nice to have consistency, but if you reformat all this stuff we kind
of lose track of what is new code, and what's just re-formatted. The
diffs are clear here in Reitveld, so maybe go for consistency. I'm open
to other reviewers suggestions.....
https://codereview.chromium.org/1144373003/diff/40001/src/mips/constants-mips.h#newcode456
src/mips/constants-mips.h:456: // SPECIAL3 Encoding of sa Field
nit: period at the end of all comments. Here and below...
https://codereview.chromium.org/1144373003/diff/40001/src/mips/constants-mips.h#newcode967
src/mips/constants-mips.h:967: // DCHECK(InstructionType() ==
kRegisterType);
Hmmm, either fix it, or delete it, we shouldn't have commented out
lines.
https://codereview.chromium.org/1144373003/diff/40001/src/mips/simulator-mips.cc
File src/mips/simulator-mips.cc (right):
https://codereview.chromium.org/1144373003/diff/40001/src/mips/simulator-mips.cc#newcode4177
src/mips/simulator-mips.cc:4177: // rt field: checking 5-bits
nits: comments end with periods, several places.
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.
