MIPS: Implemented PC-relative instructions for R6.

MIPS: Implemented PC-relative instructions for R6.

Added: JIC, BEQZC, JIALC, LDPC, LWPC, ALUIPC, ADDIUPC, ALIGN/DAILGN
Additional fixed compact branch offset.

TEST=test-assembler-mips[64]/r6_align, r6_dalign, r6_aluipc, r6_lwpc, r6_jic,
                             r6_beqzc, r6_jialc, r6_addiupc, r6_ldpc
BUG=

[ilija.pavlovic, 2015-05-29 06:35:48]

ilija.pavlovic.imgtec@gmail.com changed reviewers:
+ akos.palfi@imgtec.com, balazs.kilvady@imgtec.com,
dusan.milosavljevic@imgtec.com, paul.lind@imgtec.com

[dusmil.imgtec, 2015-06-01 09:35:28]

https://codereview.chromium.org/1144373003/diff/1/test/cctest/test-disasm-mip...
File test/cctest/test-disasm-mips.cc (right):

https://codereview.chromium.org/1144373003/diff/1/test/cctest/test-disasm-mip...
test/cctest/test-disasm-mips.cc:536: if (kArchVariant == kMips32r6) {
It should be IsMipsArchVariant() as above.

[paul.l..., 2015-06-10 03:49:26]

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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
src/mips/simulator-mips.cc:4177: // rt field: checking 5-bits
nits: comments end with periods, several places.

[paul.l..., 2015-06-11 05:56:32]

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-assemb...
File test/cctest/test-assembler-mips.cc (right):

https://codereview.chromium.org/1144373003/diff/40001/test/cctest/test-assemb...
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-assemb...
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-assemb...
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-assemb...
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-assemb...
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-assemb...
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-assemb...
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-assemb...
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-assemb...
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.

[ilija.pavlovic, 2015-06-12 09:51:33]

Performed adaptations according review comments.

Best regards
Ilija

https://codereview.chromium.org/1144373003/diff/1/test/cctest/test-disasm-mip...
File test/cctest/test-disasm-mips.cc (right):

https://codereview.chromium.org/1144373003/diff/1/test/cctest/test-disasm-mip...
test/cctest/test-disasm-mips.cc:536: if (kArchVariant == kMips32r6) {
On 2015/06/01 09:35:27, dusmil.imgtec wrote:
> It should be IsMipsArchVariant() as above.

Done.

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...
src/mips/assembler-mips.cc:285: Assembler::Assembler(Isolate* isolate, void*
buffer, int buffer_size)
On 2015/06/10 03:49:25, paul.l... wrote:
> nit: don't delete the line here, there should be 2 blank lines before
> Assembler...

Done.

https://codereview.chromium.org/1144373003/diff/40001/src/mips/assembler-mips...
src/mips/assembler-mips.cc:970: DCHECK((kMinInt26 <= offset26) && (offset26 <=
kMaxInt26));
On 2015/06/10 03:49:25, paul.l... wrote:
> You should be able to use is_int26() here - defined around line 1016 of
utils.h

Done.

https://codereview.chromium.org/1144373003/diff/40001/src/mips/assembler-mips...
src/mips/assembler-mips.cc:971: Instr instr = opcode |
static_cast<uint32_t>(offset26 & kImm26Mask);
On 2015/06/10 03:49:25, paul.l... wrote:
> I don't think you need the cast here -- all the types are uint32_t already.

Done.

https://codereview.chromium.org/1144373003/diff/40001/src/mips/assembler-mips...
src/mips/assembler-mips.cc:1180: DCHECK(IsMipsArchVariant(kMips32r6));
On 2015/06/10 03:49:25, paul.l... wrote:
> I suspect we will need positions_recorder()->WriteRecordedPositions(); 
Added "positions_recorder()->WriteRecordedPositions();".
Done.

https://codereview.chromium.org/1144373003/diff/40001/src/mips/assembler-mips...
src/mips/assembler-mips.cc:1385: static_cast<uint32_t>(offset & kImm21Mask);
On 2015/06/10 03:49:25, paul.l... wrote:
> 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.
The static cast removed here and in instances below.
Done.

https://codereview.chromium.org/1144373003/diff/40001/src/mips/assembler-mips...
src/mips/assembler-mips.cc:1801: // 'lui' has zero reg. for rs field.
On 2015/06/10 03:49:25, paul.l... wrote:
> You should add DCHECK(rs != zero_reg); 
> (I know its not your change ... but I can't help myself :)
Ok :-)))
Done.

https://codereview.chromium.org/1144373003/diff/40001/src/mips/assembler-mips...
src/mips/assembler-mips.cc:1812: DCHECK((kMinInt19 <= imm19) && (imm19 <=
kMaxInt19));
On 2015/06/10 03:49:25, paul.l... wrote:
> I think is_int19() will do, here and below.

Done.

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...
src/mips/assembler-mips.h:761: void jal_or_jalr(int32_t target, Register rs);
On 2015/06/10 03:49:25, paul.l... wrote:
> 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.

These two functions are removed.

https://codereview.chromium.org/1144373003/diff/40001/src/mips/assembler-mips...
src/mips/assembler-mips.h:761: void jal_or_jalr(int32_t target, Register rs);
On 2015/06/10 03:49:25, paul.l... wrote:
> 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.

Done.

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...
src/mips/constants-mips.h:248: const int32_t kMinInt26 = -33554432;
On 2015/06/10 03:49:25, paul.l... wrote:
> I don't think you need this constants if you change assembler to use
is_intNN()
These constants are deleted. They were actually defined for test purposes
(maximal and minimal values for an offset or an immediate).
Done.

https://codereview.chromium.org/1144373003/diff/40001/src/mips/constants-mips...
src/mips/constants-mips.h:267: const int kBp2Bits = 2;
On 2015/06/10 03:49:25, paul.l... wrote:
> 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.....
I made an effort to have nicely aligned signs "=", but after executing "git cl
format" the result is what we cane see in the source codes for this review. If
this command changes our code on this way, should we use it at all?

https://codereview.chromium.org/1144373003/diff/40001/src/mips/constants-mips...
src/mips/constants-mips.h:456: // SPECIAL3 Encoding of sa Field
On 2015/06/10 03:49:25, paul.l... wrote:
> nit: period at the end of all comments. Here and below...

Done.

https://codereview.chromium.org/1144373003/diff/40001/src/mips/constants-mips...
src/mips/constants-mips.h:967: // DCHECK(InstructionType() == kRegisterType);
On 2015/06/10 03:49:25, paul.l... wrote:
> Hmmm, either fix it, or delete it, we shouldn't have commented out lines.
Deleted.
Done.

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...
src/mips/simulator-mips.cc:4177: // rt field: checking 5-bits
On 2015/06/10 03:49:26, paul.l... wrote:
> nits: comments end with periods, several places.

Done.

https://codereview.chromium.org/1144373003/diff/40001/test/cctest/test-assemb...
File test/cctest/test-assembler-mips.cc (right):

https://codereview.chromium.org/1144373003/diff/40001/test/cctest/test-assemb...
test/cctest/test-assembler-mips.cc:4399: uint32_t PC;  // Program Counter
On 2015/06/11 05:56:32, paul.l... wrote:
> nit: period at end.

Done.

https://codereview.chromium.org/1144373003/diff/40001/test/cctest/test-assemb...
test/cctest/test-assembler-mips.cc:4418: PC = (uint32_t) f;  // set program
counter
On 2015/06/11 05:56:32, paul.l... wrote:
> 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.

Done.

https://codereview.chromium.org/1144373003/diff/40001/test/cctest/test-assemb...
test/cctest/test-assembler-mips.cc:4522: // addiu t3, a4, 0xffff;  (0x250fffff)
On 2015/06/11 05:56:32, paul.l... wrote:
> You have mips64 register names here. This is addiu t7, t0, 0xffff in mips32
reg
> naming convention. Please adjust other nearby comments as well.

Adapted names in the code comment.
addiu t7, t0, 0xffff; addiu t4, t0, 0x0000; addiu t0, t0, 0x0000; addiu t3, t0,
0xffff;

Done.

https://codereview.chromium.org/1144373003/diff/40001/test/cctest/test-assemb...
test/cctest/test-assembler-mips.cc:4573: { t8.code(),      -262144,        
0x250fffff },   // offset 0x40000
On 2015/06/11 05:56:31, paul.l... wrote:
> 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.

I tried to mimic (or follow) the format "LWPC rs, offset" in function's
signature "run_lwpc(int rs, int offset)", but you are right: - it is confusing.
Therefore, I decided to change the function signature into "run_lwpc(int
offset)", and "int rs;" in the structure definition is substituted with this
comment: "// rs - A word from memory will be stored in t8 register." I think
that this is enough descriptive.

Done.

https://codereview.chromium.org/1144373003/diff/40001/test/cctest/test-assemb...
test/cctest/test-assembler-mips.cc:4657: // rt - formal argument; will contain
value of the program counter
On 2015/06/11 05:56:32, paul.l... wrote:
> 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.
> 

Similar explanation as for LWPC. rt is removed from the function's signature and
in the structure TestCaseJic the line "uint32_t rt;" is substituted with
following comment: "As rt will be used t0 register which will have value of the
program counter for the jic instruction."

Done.

https://codereview.chromium.org/1144373003/diff/40001/test/cctest/test-assemb...
test/cctest/test-assembler-mips.cc:4660: //{  0,         0,            0x0 }, 
// JIC in loop
On 2015/06/11 05:56:32, paul.l... wrote:
> Don't leave in the commented out case.
The line is deleted.

Done.

https://codereview.chromium.org/1144373003/diff/40001/test/cctest/test-assemb...
test/cctest/test-assembler-mips.cc:4726: uint32_t  rs;
On 2015/06/11 05:56:32, paul.l... wrote:
> 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.

The parameter 'rt' renamed into 'rt_value'.

Done.

https://codereview.chromium.org/1144373003/diff/40001/test/cctest/test-assemb...
test/cctest/test-assembler-mips.cc:4826: // rt - formal argument; will contain
value of the program counter
On 2015/06/11 05:56:31, paul.l... wrote:
> Again, rt is unused here, I suggest removing it for clarity. Same comment
> applies to other tests also (like the next one).

Corrections performed similarly previous test cases.

Done.

https://codereview.chromium.org/1144373003/diff/40001/test/cctest/test-assemb...
test/cctest/test-assembler-mips.cc:4913: for (int32_t i = -100; i <= -11; ++i) {
On 2015/06/11 05:56:32, paul.l... wrote:
> 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.

Actually, in the first version this test case had minimum and maximum values for
the offset (instead -100 it was kMinInt26, and instead 99 it was kMaxInt26). It
generated really enough instructions for complete testing "bc" and "balc", but
there were problems:
- For mips32r6.release on the simulator it was not possible to execute because
of some problems with allocation memory.
- For mips64r6.release on the simulator the test was successful, but test
execution time was ~3 minutes.
- For mips64.release on the qemu test execution had problems. For example, if
the first test case is executed, the qemu needs certain time for releasing the
memory for next test case (~ 15 seconds or more). If the second test starts
immediately, very soon will appear a message about problems with allocation
memory. The test case where the offset is set on kMinInt26, was not successful;
again the problem with memory.

Because of mentioned limits of the simulators, I decided to define test cases
with small ranges of the offset.

Done.

[dusmil.imgtec, 2015-06-12 10:11:43]

LGTM.

[paul.l..., 2015-06-15 04:57:51]

All the comments in test-assembler-mips64.cc likely also apply to
test-assembler-mips.cc also. Sorry, I was somewhat in-accurate in my previous
review comments on that file, and I may have misled you a bit about the runtime
vs generation time issues. I think it is clear now.

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
File test/cctest/test-assembler-mips64.cc (right):

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:4450: uint64_t run_align(uint64_t rs,
uint64_t rt, uint8_t bp) {
I explain in more detail below -- but I don't like these name rs, rt here, since
you are not using them to specify registers in the generated code, but rather
just values that are passed thru the function-call ABI (in regs a0, a1).

bp is used for code-generation, and is named fine, but should not be in the
runtime call.

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:4468: uint64_t rd =
Also explained in more detail below, this is not "really" rd. It the result
being returned from a function, which per ABI is register v0 -- which you've
hardcoded above into the instruction.

Please more complete explanation in code for run_lwupc(), and adjust this
function and all the others with this expectation.

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:4487: // rd_expected,               rs,    
     rt,   bp
As below, object to these (register) names here for what is expected-result, and
input data.

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:4633: uint64_t rs =
nit: I'm not comfortable with this use of 'rs' here - I think it confuses the
use of the 'rs' register field of the instruction, with the returned 'result' of
the assembly code fragment above. By the mips ABI, the result of a function call
is returned in register v0, regardless of what instruction you use above or what
the value of the 'rs' field in a particular instruction is. Better to 'result'
or something like that. Next function, too.

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:4647: // As rs register will be used v0
register.
nit: I also think 'imm16' is not an optimal name for this field. Something like
'offset' or 'pc_offset' is more descriptive of what you are doing, rather the
repeating the name of the field within the raw instruction. (this applies to
several other of these functions as well.

I think both lines of this comment should be removed (or at least re-worded)
once the naming changes are done.

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:4722: // rs - A word from memory will be
stored in t8 register.
I think this comment no longer applies, and should be deleted.

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:4724: uint64_t  expected_rs;
nit: I think this should be named expected_res, or expected_result. It seems
slightly confusing with use of 'rs' as a register number of an intstruction.

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:4748: uint64_t run_lwupc(int rs, int
offset) {
I made an incorrect statement in patchset 3 review of test-assembler-mips.cc
TEST(r6_lwpc) -- about code-generation time vs runtime. I was partly correct,
let me try to explain more clearly what I was trying to say there, in the
context of this function, and its caller.

This function takes 'rs' as a parameter from the caller. It absolutely CAN use
that 'rs' value to generate the assembly code. However, you do not do that here,
since you hard-code the register: __ lwupc(t8, offset);

That could be written: __ lwupc(rs, offset); and it would be correct. (However,
from the caller: TEST(r6_lwupc), you never vary that register, it is always
t8.code(), so there is no point to having the parameter. If you did change to
varying registers, you would also have use __ mov(v0, rs); . That would also be
fine. Also, note that you are _generating_ the instruction with an encoded value
of 'offset', which again is fine.

What looks very wrong to me is the CALL_GENERATED_CODE(f, rs, offset, ...) where
you pass in rs, and offset at the runtime. Now you don't use these at runtime,
but you should not pass them in, because it is confusing to the reader.

Please take a look at all your test functions, to be sure there is not confusion
between generation-time, and run-time.

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:4807: { t8.code(),           -4,        
0x250c0003 },
As mentioned above, if you're going to pass in a register number to use, you
might as well vary it from call to call -- just for fun.

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:4873:
reinterpret_cast<uint64_t>(CALL_GENERATED_CODE(f, offset, 0, 0, 0, 0));
As above: the value of offset is used at generation time, and cannot (and is
not) used when the function is called. Please remove it here.

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:4885: // the program counter for the jic
instruction.
nit: this comment is not needed. The code above is clear that the input to jic
instr is t0.

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:4906: uint64_t run_beqzc(int32_t rs,
int32_t offset) {
The first parameter here is used as a 'value' that you pass to the function thru
ABI first parameter register, which is a0, and is hard-coded in the beqzc
instruction. You are not really passing in a 'rs' field, so I suggest not
calling it that.

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:4945:
reinterpret_cast<uint64_t>(CALL_GENERATED_CODE(f, rs, offset, 0, 0, 0));
You are passing in a 'value' as first param, please rename that param. You
cannot pass offset at runtime, so please make the 2nd param 0.

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:4962: // rs_value,    offset,  
expected_res
I suggest you just call this value - since you have hard-coded the 'rs' field of
the instruction to a0, this has nothing really to do with 'rs'.

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:5105: // As rs register will be used v0
register.
nit: please delete this comment. I'm not sure what the first line is trying to
say, and the 2nd line about v0 is hard-coded above, and is clear.

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:5146: __ mov(v0, t8);
As above: If you are passing in rs, I suggest you use it here, rather than
hard-code t8.

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:5169:
reinterpret_cast<uint64_t>(CALL_GENERATED_CODE(f, rs, offset, 0, 0, 0));
As above, you cannot use rs or offset at runtime, just pass 0.

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:5272: {          0,   (abs(-100) - 10 + 1 +
99) },
nit; weird brace alignment, if you can line them up, great, if clang-format
prevents, then do the entire table without the extra spaces.

[ilija.pavlovic, 2015-06-15 10:40:11]

Updated according review comments for the Patch Set 4.

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
File test/cctest/test-assembler-mips64.cc (right):

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:4450: uint64_t run_align(uint64_t rs,
uint64_t rt, uint8_t bp) {
On 2015/06/15 04:57:50, paul.l... wrote:
> I explain in more detail below -- but I don't like these name rs, rt here,
since
> you are not using them to specify registers in the generated code, but rather
> just values that are passed thru the function-call ABI (in regs a0, a1).
> 
> bp is used for code-generation, and is named fine, but should not be in the
> runtime call.

Parameter names changed: rs -> rs_value, rt -> rt_value.

Done.

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:4468: uint64_t rd =
On 2015/06/15 04:57:51, paul.l... wrote:
> Also explained in more detail below, this is not "really" rd. It the result
> being returned from a function, which per ABI is register v0 -- which you've
> hardcoded above into the instruction.
> 
> Please more complete explanation in code for run_lwupc(), and adjust this
> function and all the others with this expectation.

Variable name "rd" substituted with "res". "bp" removed from
"CALL_GENERATED_CODE".

Done.

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:4487: // rd_expected,               rs,    
     rt,   bp
On 2015/06/15 04:57:50, paul.l... wrote:
> As below, object to these (register) names here for what is expected-result,
and
> input data.

Following substitutions are performed: rs -> rs_value, rt -> rt_value,
rd_expected -> expected_res. 
The order for defined values is: rs_value, rt_value, bp, expected_res.
The same modification are performed also on run_dalign and r6_dalign.

Done.

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:4633: uint64_t rs =
On 2015/06/15 04:57:50, paul.l... wrote:
> nit: I'm not comfortable with this use of 'rs' here - I think it confuses the
> use of the 'rs' register field of the instruction, with the returned 'result'
of
> the assembly code fragment above. By the mips ABI, the result of a function
call
> is returned in register v0, regardless of what instruction you use above or
what
> the value of the 'rs' field in a particular instruction is. Better to 'result'
> or something like that. Next function, too.

I agree. "rs" is substituted with "res".

Done.

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:4647: // As rs register will be used v0
register.
On 2015/06/15 04:57:50, paul.l... wrote:
> nit: I also think 'imm16' is not an optimal name for this field. Something
like
> 'offset' or 'pc_offset' is more descriptive of what you are doing, rather the
> repeating the name of the field within the raw instruction. (this applies to
> several other of these functions as well.
> 
> I think both lines of this comment should be removed (or at least re-worded)
> once the naming changes are done.

"imm16" is substituted with "offset".
The comment line is removed.

Done.

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:4722: // rs - A word from memory will be
stored in t8 register.
On 2015/06/15 04:57:50, paul.l... wrote:
> I think this comment no longer applies, and should be deleted.

The comment deleted.

Done.

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:4724: uint64_t  expected_rs;
On 2015/06/15 04:57:50, paul.l... wrote:
> nit: I think this should be named expected_res, or expected_result. It seems
> slightly confusing with use of 'rs' as a register number of an intstruction.

"expected_rs" is substituted with "expected_res".

Done.

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:4748: uint64_t run_lwupc(int rs, int
offset) {
On 2015/06/15 04:57:50, paul.l... wrote:
> I made an incorrect statement in patchset 3 review of test-assembler-mips.cc
> TEST(r6_lwpc) -- about code-generation time vs runtime. I was partly correct,
> let me try to explain more clearly what I was trying to say there, in the
> context of this function, and its caller.
> 
> This function takes 'rs' as a parameter from the caller. It absolutely CAN use
> that 'rs' value to generate the assembly code. However, you do not do that
here,
> since you hard-code the register: __ lwupc(t8, offset);
> 
> That could be written: __ lwupc(rs, offset); and it would be correct.
(However,
> from the caller: TEST(r6_lwupc), you never vary that register, it is always
> t8.code(), so there is no point to having the parameter. If you did change to
> varying registers, you would also have use __ mov(v0, rs); . That would also
be
> fine. Also, note that you are _generating_ the instruction with an encoded
value
> of 'offset', which again is fine.
> 
> What looks very wrong to me is the CALL_GENERATED_CODE(f, rs, offset, ...)
where
> you pass in rs, and offset at the runtime. Now you don't use these at runtime,
> but you should not pass them in, because it is confusing to the reader.
> 
> Please take a look at all your test functions, to be sure there is not
confusion
> between generation-time, and run-time.

Thanks for really detail explanations. You see that I tried to keep some kind of
(really not needed) formalism. I will try to apply you suggestions on the rest
of the code. In this place, "rs" parameter is removed.

Done.

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:4807: { t8.code(),           -4,        
0x250c0003 },
On 2015/06/15 04:57:51, paul.l... wrote:
> As mentioned above, if you're going to pass in a register number to use, you
> might as well vary it from call to call -- just for fun.

These entries are deleted.

Done.

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:4873:
reinterpret_cast<uint64_t>(CALL_GENERATED_CODE(f, offset, 0, 0, 0, 0));
On 2015/06/15 04:57:50, paul.l... wrote:
> As above: the value of offset is used at generation time, and cannot (and is
> not) used when the function is called. Please remove it here.

"offset" removed from "CALL_GENERATED_CODE".

Done.

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:4885: // the program counter for the jic
instruction.
On 2015/06/15 04:57:50, paul.l... wrote:
> nit: this comment is not needed. The code above is clear that the input to jic
> instr is t0.

The comment deleted.

Done.

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:4906: uint64_t run_beqzc(int32_t rs,
int32_t offset) {
On 2015/06/15 04:57:50, paul.l... wrote:
> The first parameter here is used as a 'value' that you pass to the function
thru
> ABI first parameter register, which is a0, and is hard-coded in the beqzc
> instruction. You are not really passing in a 'rs' field, so I suggest not
> calling it that.
"rs" renamed into "value". 

Done.

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:4945:
reinterpret_cast<uint64_t>(CALL_GENERATED_CODE(f, rs, offset, 0, 0, 0));
On 2015/06/15 04:57:50, paul.l... wrote:
> You are passing in a 'value' as first param, please rename that param. You
> cannot pass offset at runtime, so please make the 2nd param 0.

"rs" renamed as "value" and "offset" is substituted with "0".

Done.

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:4962: // rs_value,    offset,  
expected_res
On 2015/06/15 04:57:51, paul.l... wrote:
> I suggest you just call this value - since you have hard-coded the 'rs' field
of
> the instruction to a0, this has nothing really to do with 'rs'.

"rs_value" renamed into "value".

Done.

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:5105: // As rs register will be used v0
register.
On 2015/06/15 04:57:50, paul.l... wrote:
> nit: please delete this comment. I'm not sure what the first line is trying to
> say, and the 2nd line about v0 is hard-coded above, and is clear.

The comment deleted. Also, "rs" renamed into "res", and "rs_expected" into
"expected_rs".

Done.

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:5146: __ mov(v0, t8);
On 2015/06/15 04:57:50, paul.l... wrote:
> As above: If you are passing in rs, I suggest you use it here, rather than
> hard-code t8.

"rs" removed from the parameter list.

Done.

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:5169:
reinterpret_cast<uint64_t>(CALL_GENERATED_CODE(f, rs, offset, 0, 0, 0));
On 2015/06/15 04:57:50, paul.l... wrote:
> As above, you cannot use rs or offset at runtime, just pass 0.

"rs" and "offset" are substituted with "0".

Done.

https://codereview.chromium.org/1144373003/diff/60001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:5272: {          0,   (abs(-100) - 10 + 1 +
99) },
On 2015/06/15 04:57:51, paul.l... wrote:
> nit; weird brace alignment, if you can line them up, great, if clang-format
> prevents, then do the entire table without the extra spaces.

Corrected.

Done.

[paul.l..., 2015-06-16 01:55:50]

LGTM, but it needs to be rebased for current master.

[dusmil.imgtec, 2015-06-17 14:24:13]

https://codereview.chromium.org/1144373003/diff/120001/src/mips64/simulator-m...
File src/mips64/simulator-mips64.cc (right):

https://codereview.chromium.org/1144373003/diff/120001/src/mips64/simulator-m...
src/mips64/simulator-mips64.cc:4498: set_register(static_cast<int32_t>(rs_reg),
alu_out);
Instead of cast, change rs_reg type to be int32_t.

[ilija.pavlovic, 2015-06-18 05:32:51]

Updated fix -Wshorten warnings.

https://codereview.chromium.org/1144373003/diff/120001/src/mips64/simulator-m...
File src/mips64/simulator-mips64.cc (right):

https://codereview.chromium.org/1144373003/diff/120001/src/mips64/simulator-m...
src/mips64/simulator-mips64.cc:4498: set_register(static_cast<int32_t>(rs_reg),
alu_out);
On 2015/06/17 14:24:13, dusmil.imgtec wrote:
> Instead of cast, change rs_reg type to be int32_t.

Done.

[dusmil.imgtec, 2015-06-18 09:45:02]

Final LGTM.

[ilija.pavlovic, 2015-06-18 10:15:34]

The CQ bit was checked by ilija.pavlovic.imgtec@gmail.com

[ilija.pavlovic, 2015-06-18 10:15:34]

The patchset sent to the CQ was uploaded after l-g-t-m from paul.lind@imgtec.com
Link to the patchset: https://codereview.chromium.org/1144373003/#ps140001
(title: "Updated fix -Wshorten warnings.")

[commit-bot: I haz the power, 2015-06-18 10:15:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1144373003/140001

[commit-bot: I haz the power, 2015-06-18 10:19:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-18 10:19:03]

Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/3498)

[dusmil.imgtec, 2015-06-18 15:16:46]

Re-upload the CL with imgtec account.

[dusmil.imgtec, 2015-06-19 12:28:18]

Landed in: https://codereview.chromium.org/1195793002/
Please close.