Looks very good, mostly my comments are about nits, but there is one real
issue
with jr/jalr() I think.
https://codereview.chromium.org/426863006/diff/1/src/mips64/assembler-mips64.cc
File src/mips64/assembler-mips64.cc (right):
https://codereview.chromium.org/426863006/diff/1/src/mips64/assembler-mips64.cc#newcode1392
src/mips64/assembler-mips64.cc:1392: jalr(rs, at);
I think this should be jalr(rs, zero_reg)
Also, jalr() itself does the WriteRecordedPositions() and the
BlockTrampolinePoolFor(1) so you should probably rearrange the if
statement to exclude those.
https://codereview.chromium.org/426863006/diff/1/src/mips64/assembler-mips64.cc#newcode1911
src/mips64/assembler-mips64.cc:1911: GenInstrImmediate(LUI, rs, rt, j);
A comment here about LUI/AUI and zero_reg would be helpful, though I
suppose one could just look at previous instruction ;)
https://codereview.chromium.org/426863006/diff/1/src/mips64/assembler-mips64.cc#newcode2521
src/mips64/assembler-mips64.cc:2521: ASSERT(is_uint3(cc));
Maybe ASSERT(kArchVariant != kMips64r6) here?
https://codereview.chromium.org/426863006/diff/1/src/mips64/constants-mips64.h
File src/mips64/constants-mips64.h (right):
https://codereview.chromium.org/426863006/diff/1/src/mips64/constants-mips64.h#newcode20
src/mips64/constants-mips64.h:20: kMips64r1,
I do not think we need kMips64r1 -- there are only 2 other usages, and
it seems they can be skipped.
https://codereview.chromium.org/426863006/diff/1/src/mips64/constants-mips64.h#newcode419
src/mips64/constants-mips64.h:419: MUL_MUH = ((3 << 3) + 0), //
MUL, MUH.
nit: one space after MUL,
https://codereview.chromium.org/426863006/diff/1/src/mips64/disasm-mips64.cc
File src/mips64/disasm-mips64.cc (right):
https://codereview.chromium.org/426863006/diff/1/src/mips64/disasm-mips64.cc#newcode675
src/mips64/disasm-mips64.cc:675: case D_MUL_MUH: // Eqauls to DMUL.
nit: Eqauls -> Equals
https://codereview.chromium.org/426863006/diff/1/src/mips64/macro-assembler-mips64.cc
File src/mips64/macro-assembler-mips64.cc (right):
https://codereview.chromium.org/426863006/diff/1/src/mips64/macro-assembler-mips64.cc#newcode761
src/mips64/macro-assembler-mips64.cc:761: // dmul(rd, rs, rt.rm());
I think we can now remove this TODO, and the one just below.
https://codereview.chromium.org/426863006/diff/1/src/mips64/simulator-mips64.cc
File src/mips64/simulator-mips64.cc (right):
https://codereview.chromium.org/426863006/diff/1/src/mips64/simulator-mips64.cc#newcode2086
src/mips64/simulator-mips64.cc:2086: case MULT: // MULT == D_MUL_MUH
nit: end comment with period, here and DMULT below.
https://codereview.chromium.org/426863006/diff/1/src/mips64/simulator-mips64.cc#newcode2660
src/mips64/simulator-mips64.cc:2660: case DMULT: // DMULT == D_MUL_MUH
nit: trailing period on comment.
https://codereview.chromium.org/426863006/
--
--
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.
