MIPS: Fix trampoline pool handling in MacroAssembler::BranchShort() for r6.

MIPS: Fix trampoline pool handling in MacroAssembler::BranchShort() for r6.

BUG=chromium:555543
LOG=N

Committed: https://crrev.com/c91bcf719208f12a88f6c9fcf6d73204762739f2
Cr-Commit-Position: refs/heads/master@{#32094}

[balazs.kilvady, 2015-11-17 14:01:47]

Patchset #2 (id:20001) has been deleted

[balazs.kilvady, 2015-11-17 14:12:39]

Description was changed from

==========
MIPS: Fix trampoline pool handling in MacroAssembler::BranchShort() for r6.

BUG=chromium:555543
LOG=N
==========

to

==========
MIPS: Fix trampoline pool handling in MacroAssembler::BranchShort() for r6.

BUG=chromium:555543
LOG=N
==========

[balazs.kilvady, 2015-11-17 14:12:39]

balazs.kilvady@imgtec.com changed reviewers:
+ akos.palfi@imgtec.com, hablich@chromium.org, paul.lind@imgtec.com

[paul.l..., 2015-11-17 19:59:45]

https://codereview.chromium.org/1446343002/diff/40001/src/mips/assembler-mips.cc
File src/mips/assembler-mips.cc (right):

https://codereview.chromium.org/1446343002/diff/40001/src/mips/assembler-mips...
src/mips/assembler-mips.cc:1473: jalr(rs, zero_reg);
Don't we need this protected as well?

https://codereview.chromium.org/1446343002/diff/40001/src/mips64/assembler-mi...
File src/mips64/assembler-mips64.cc (right):

https://codereview.chromium.org/1446343002/diff/40001/src/mips64/assembler-mi...
src/mips64/assembler-mips64.cc:1428: j(imm);
Doesn't this unbound case also need to be protected? Also for jal and jr below.
If so, these could be moved outside the if block, so they only appear once.

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

https://codereview.chromium.org/1446343002/diff/40001/test/cctest/test-assemb...
test/cctest/test-assembler-mips.cc:5106: if (IsMipsArchVariant(kMips32r6)) {
Does this test have to be r6-only? Can we write a test that hits the boundary
cases for both r2 & r6? (for mips64 as well).

[balazs.kilvady, 2015-11-18 12:22:53]

https://codereview.chromium.org/1446343002/diff/40001/src/mips/assembler-mips.cc
File src/mips/assembler-mips.cc (right):

https://codereview.chromium.org/1446343002/diff/40001/src/mips/assembler-mips...
src/mips/assembler-mips.cc:1473: jalr(rs, zero_reg);
On 2015/11/17 19:59:45, paul.l... wrote:
> Don't we need this protected as well?

We don't need protection here as jalr() contains the protection code.

https://codereview.chromium.org/1446343002/diff/40001/src/mips64/assembler-mi...
File src/mips64/assembler-mips64.cc (right):

https://codereview.chromium.org/1446343002/diff/40001/src/mips64/assembler-mi...
src/mips64/assembler-mips64.cc:1428: j(imm);
On 2015/11/17 19:59:45, paul.l... wrote:
> Doesn't this unbound case also need to be protected? Also for jal and jr
below.
> If so, these could be moved outside the if block, so they only appear once.

We should not add protection here as implementation of j() contains protection.

Other thing: I checked that parameter of j should be int32_t, not 64 bits. I did
not put that changes into this CL but would be fine to fix it.

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

https://codereview.chromium.org/1446343002/diff/40001/test/cctest/test-assemb...
test/cctest/test-assembler-mips.cc:5106: if (IsMipsArchVariant(kMips32r6)) {
On 2015/11/17 19:59:45, paul.l... wrote:
> Does this test have to be r6-only? Can we write a test that hits the boundary
> cases for both r2 & r6? (for mips64 as well).

I am thinking about it. Would be fine.

[balazs.kilvady, 2015-11-18 14:04:05]

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

https://codereview.chromium.org/1446343002/diff/40001/test/cctest/test-assemb...
test/cctest/test-assembler-mips.cc:5106: if (IsMipsArchVariant(kMips32r6)) {
On 2015/11/18 12:22:53, balazs.kilvady wrote:
> On 2015/11/17 19:59:45, paul.l... wrote:
> > Does this test have to be r6-only? Can we write a test that hits the
boundary
> > cases for both r2 & r6? (for mips64 as well).
> 
> I am thinking about it. Would be fine.

Fortunately this test case forces trampoline generation on all the MIPS revs so
I uploaded a patchiest to fix/enable it.
Done.

[paul.l..., 2015-11-18 16:47:15]

LGTM, thanks!

[balazs.kilvady, 2015-11-18 17:03:37]

The CQ bit was checked by balazs.kilvady@imgtec.com

[commit-bot: I haz the power, 2015-11-18 17:03:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1446343002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1446343002/60001

[commit-bot: I haz the power, 2015-11-18 17:43:09]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-11-18 17:43:47]

Patchset 3 (id:??) landed as
https://crrev.com/c91bcf719208f12a88f6c9fcf6d73204762739f2
Cr-Commit-Position: refs/heads/master@{#32094}