MIPS: r6 compact branch optimization.

MIPS: r6 compact branch optimization.

BUG=

Committed: https://crrev.com/3573d3cb58bffb08d0537eaa3803fb5c2a5e5639
Cr-Commit-Position: refs/heads/master@{#31761}

[balazs.kilvady, 2015-10-12 11:51:17]

Patchset #3 (id:40001) has been deleted

[balazs.kilvady, 2015-10-13 18:39:49]

balazs.kilvady@imgtec.com changed reviewers:
+ paul.lind@imgtec.com

[balazs.kilvady, 2015-10-14 13:42:47]

Patchset #8 (id:160001) has been deleted

[paul.l..., 2015-10-15 01:51:23]

paul.lind@imgtec.com changed reviewers:
+ djordje.pesic@imgtec.com, ivica.bogosavljevic@imgtec.com

[paul.l..., 2015-10-15 01:51:24]

This is looking really good. Some nice ideas in here, especiaily in the sim.
Several of my comments should be discussed a bit, maybe we can have a skype
call? 

Ivica, Djordje, can you guys have a look as well?

https://codereview.chromium.org/1396133002/diff/180001/src/ic/mips/ic-mips.cc
File src/ic/mips/ic-mips.cc (right):

https://codereview.chromium.org/1396133002/diff/180001/src/ic/mips/ic-mips.cc...
src/ic/mips/ic-mips.cc:884: Assembler::instr_at(patch_address +
Instruction::kInstrSize);
Add ' USE(branch_instr);' here or somewhere. After the change, all usages are in
DCHECK, so release builds give unused var warning/error.

https://codereview.chromium.org/1396133002/diff/180001/src/ic/mips/ic-mips.cc...
src/ic/mips/ic-mips.cc:905: patcher.ChangeBranchCondition();
I kind of like the old way, where you check if (eq) set-to(ne), etc.

What you have now is a just a toggle. Could this get out of sync in any way?

(I do understand extra complexity in the implementation of
ChangeBranchCondition())

https://codereview.chromium.org/1396133002/diff/180001/src/mips/assembler-mip...
File src/mips/assembler-mips-inl.h (right):

https://codereview.chromium.org/1396133002/diff/180001/src/mips/assembler-mip...
src/mips/assembler-mips-inl.h:453: BlockTrampolinePoolFor(1);  // For saving
forbidden slot.
Doesn't this BlockTrampolinePoolFor(1) come too late to work here? 

You just did a CheckBuffer() above, which could emit a trampoline. I think you
need to move this block above that one.

https://codereview.chromium.org/1396133002/diff/180001/src/mips/assembler-mip...
src/mips/assembler-mips-inl.h:455: ->IsForbiddenInBranchDelay()) {
I think the use of 'Delay' in this name is mis-leading. We have two similar
concepts, regular (old-style) branches do have Branch Delay slots, and they are
present in all revs including r6. Compact Branches do not have delay slots, they
have forbidden slots. This still is all plenty confusing already, and I think we
must be extra-careful with our naming and comments, to help keep this as clear
as possible.

In this case, you are _only_ in this block after a compact branch, and so by
definition, you do NOT have a delay slot.

Suggest you call this IsForbiddenAfterBranch() or
IsForbiddenAfterCompactBranch()

https://codereview.chromium.org/1396133002/diff/180001/src/mips/macro-assembl...
File src/mips/macro-assembler-mips.cc (right):

https://codereview.chromium.org/1396133002/diff/180001/src/mips/macro-assembl...
src/mips/macro-assembler-mips.cc:5705: // would be tricky.
This comment block is a little out of date. Same basic idea, but you might add
something about tricky encoding of r6 compact branches makes this simple idea
have a non-trivial implementation.

https://codereview.chromium.org/1396133002/diff/180001/src/mips/macro-assembl...
src/mips/macro-assembler-mips.cc:5717: opcode = POP30;
Even with the comments above, I really don't like all this POP junk. Maybe just
add the comments locally for clarity:

  opecode = POP30;  // change BEQC to BNEC.

https://codereview.chromium.org/1396133002/diff/180001/src/mips/simulator-mip...
File src/mips/simulator-mips.cc (right):

https://codereview.chromium.org/1396133002/diff/180001/src/mips/simulator-mip...
src/mips/simulator-mips.cc:3769: // Branch instructions common part.
lambda functions! Cool.... reads nicely from the caller.

https://codereview.chromium.org/1396133002/diff/180001/src/mips/simulator-mip...
src/mips/simulator-mips.cc:3872: case POP06:
even though the actual instruction names are just below, I wonder if it would
make these _terrible_ opcode names easier to read if you just add the list of
supported instructions as a comment:

case POP06:  // BLEZALC, BGEUC, BLEZ (r2)

Or do you think since POP06 is an alias of BLEZ, it might make sense to do this:

case BLEZ:  //  includes mips32r6 BLEZALC, BGEUC

Maybe mention the funny-name opcode for cross-reference

case BLEZ:  // mips32r6 BLEZALC, BGEUC (opcode group POP06)

Let's discuss this one more .....

https://codereview.chromium.org/1396133002/diff/180001/src/mips/simulator-mips.h
File src/mips/simulator-mips.h (right):

https://codereview.chromium.org/1396133002/diff/180001/src/mips/simulator-mip...
src/mips/simulator-mips.h:352: Instruction* branch_delay_instr =
nit: Improper use of 'delay' in name: this should be instr_after_branch or
instr_aftter_compact_branch, or inst_after_cb.

[ivica.bogosavljevic, 2015-10-15 11:49:08]

https://codereview.chromium.org/1396133002/diff/180001/src/mips/assembler-mip...
File src/mips/assembler-mips.cc (right):

https://codereview.chromium.org/1396133002/diff/180001/src/mips/assembler-mip...
src/mips/assembler-mips.cc:675: return 26;
Use contstants instead of number

https://codereview.chromium.org/1396133002/diff/180001/src/mips/assembler-mip...
src/mips/assembler-mips.cc:730: DCHECK(IsOri(instr_ori));
Since isLui is not part of else if, then we should DCHECK(IsLui(instr_lui));
here

https://codereview.chromium.org/1396133002/diff/180001/src/mips/assembler-mip...
src/mips/assembler-mips.cc:787: DCHECK(IsOri(instr_ori));
Since isLui is not part of else if, then we should DCHECK(IsLui(instr_lui));here

https://codereview.chromium.org/1396133002/diff/180001/src/mips/assembler-mip...
src/mips/assembler-mips.cc:1081: int32_t Assembler::branch_offset_helper(Label*
L, bool jump_elimination_allowed,
Bravo for removing all these functions!

https://codereview.chromium.org/1396133002/diff/180001/src/mips/assembler-mips.h
File src/mips/assembler-mips.h (right):

https://codereview.chromium.org/1396133002/diff/180001/src/mips/assembler-mip...
src/mips/assembler-mips.h:421: return branch_offset_helper(L,
jump_elimination_allowed, 16);
It would be nice to have these three functions inlined (though I am not sure if
we profit with this): branch_offset, branch_offset21 and branch_offset26.
Also, the numbers 16, 21 and 26 should be replaced with named constants

https://codereview.chromium.org/1396133002/diff/180001/src/mips/assembler-mip...
src/mips/assembler-mips.h:579: void b(Label* L) { b(shifted_branch_offset(L,
false)); }
Mark inline small functions

https://codereview.chromium.org/1396133002/diff/180001/src/mips/assembler-mip...
src/mips/assembler-mips.h:1392: void EmittedCompactBranchInstruction() {
prev_instr_compact_branch_ = true; }
The names of the functions EmittedCompactBranchInstruction and should be paired
for clarity ClearCompactBranchState, example:
MarkCompactBranchInstructionEmitted
ClearCompactBranchInstructionEmitted

https://codereview.chromium.org/1396133002/diff/180001/src/mips/macro-assembl...
File src/mips/macro-assembler-mips.cc (right):

https://codereview.chromium.org/1396133002/diff/180001/src/mips/macro-assembl...
src/mips/macro-assembler-mips.cc:2504: offset = GetOffset(offset, L, 16);
16 -> constant. it applies elsewhere as well

[balazs.kilvady, 2015-10-30 21:11:15]

https://codereview.chromium.org/1396133002/diff/180001/src/ic/mips/ic-mips.cc
File src/ic/mips/ic-mips.cc (right):

https://codereview.chromium.org/1396133002/diff/180001/src/ic/mips/ic-mips.cc...
src/ic/mips/ic-mips.cc:884: Assembler::instr_at(patch_address +
Instruction::kInstrSize);
On 2015/10/15 01:51:24, paul.l... wrote:
> Add ' USE(branch_instr);' here or somewhere. After the change, all usages are
in
> DCHECK, so release builds give unused var warning/error.

Done.

https://codereview.chromium.org/1396133002/diff/180001/src/ic/mips/ic-mips.cc...
src/ic/mips/ic-mips.cc:905: patcher.ChangeBranchCondition();
On 2015/10/15 01:51:24, paul.l... wrote:
> I kind of like the old way, where you check if (eq) set-to(ne), etc.
> 
> What you have now is a just a toggle. Could this get out of sync in any way?
> 
> (I do understand extra complexity in the implementation of
> ChangeBranchCondition())

Done.

https://codereview.chromium.org/1396133002/diff/180001/src/mips/assembler-mip...
File src/mips/assembler-mips-inl.h (right):

https://codereview.chromium.org/1396133002/diff/180001/src/mips/assembler-mip...
src/mips/assembler-mips-inl.h:453: BlockTrampolinePoolFor(1);  // For saving
forbidden slot.
On 2015/10/15 01:51:24, paul.l... wrote:
> Doesn't this BlockTrampolinePoolFor(1) come too late to work here? 
> 
> You just did a CheckBuffer() above, which could emit a trampoline. I think you
> need to move this block above that one.

Done.

https://codereview.chromium.org/1396133002/diff/180001/src/mips/assembler-mip...
src/mips/assembler-mips-inl.h:455: ->IsForbiddenInBranchDelay()) {
On 2015/10/15 01:51:24, paul.l... wrote:
> I think the use of 'Delay' in this name is mis-leading. We have two similar
> concepts, regular (old-style) branches do have Branch Delay slots, and they
are
> present in all revs including r6. Compact Branches do not have delay slots,
they
> have forbidden slots. This still is all plenty confusing already, and I think
we
> must be extra-careful with our naming and comments, to help keep this as clear
> as possible.
> 
> In this case, you are _only_ in this block after a compact branch, and so by
> definition, you do NOT have a delay slot.
> 
> Suggest you call this IsForbiddenAfterBranch() or
> IsForbiddenAfterCompactBranch()

Done.

https://codereview.chromium.org/1396133002/diff/180001/src/mips/assembler-mip...
File src/mips/assembler-mips.cc (right):

https://codereview.chromium.org/1396133002/diff/180001/src/mips/assembler-mip...
src/mips/assembler-mips.cc:675: return 26;
On 2015/10/15 11:49:08, ivica.bogosavljevic wrote:
> Use contstants instead of number

Done.

https://codereview.chromium.org/1396133002/diff/180001/src/mips/assembler-mip...
src/mips/assembler-mips.cc:730: DCHECK(IsOri(instr_ori));
On 2015/10/15 11:49:08, ivica.bogosavljevic wrote:
> Since isLui is not part of else if, then we should DCHECK(IsLui(instr_lui));
> here

That check is covered at line 724

https://codereview.chromium.org/1396133002/diff/180001/src/mips/assembler-mip...
src/mips/assembler-mips.cc:787: DCHECK(IsOri(instr_ori));
On 2015/10/15 11:49:08, ivica.bogosavljevic wrote:
> Since isLui is not part of else if, then we should
DCHECK(IsLui(instr_lui));here

Check covered at line 780

https://codereview.chromium.org/1396133002/diff/180001/src/mips/assembler-mips.h
File src/mips/assembler-mips.h (right):

https://codereview.chromium.org/1396133002/diff/180001/src/mips/assembler-mip...
src/mips/assembler-mips.h:421: return branch_offset_helper(L,
jump_elimination_allowed, 16);
On 2015/10/15 11:49:08, ivica.bogosavljevic wrote:
> It would be nice to have these three functions inlined (though I am not sure
if
> we profit with this): branch_offset, branch_offset21 and branch_offset26.
> Also, the numbers 16, 21 and 26 should be replaced with named constants

Done.

https://codereview.chromium.org/1396133002/diff/180001/src/mips/assembler-mip...
src/mips/assembler-mips.h:579: void b(Label* L) { b(shifted_branch_offset(L,
false)); }
On 2015/10/15 11:49:08, ivica.bogosavljevic wrote:
> Mark inline small functions

Done.

https://codereview.chromium.org/1396133002/diff/180001/src/mips/assembler-mip...
src/mips/assembler-mips.h:1392: void EmittedCompactBranchInstruction() {
prev_instr_compact_branch_ = true; }
On 2015/10/15 11:49:08, ivica.bogosavljevic wrote:
> The names of the functions EmittedCompactBranchInstruction and should be
paired
> for clarity ClearCompactBranchState, example:
> MarkCompactBranchInstructionEmitted
> ClearCompactBranchInstructionEmitted

I don't agree, please discuss it with Paul.

https://codereview.chromium.org/1396133002/diff/180001/src/mips/macro-assembl...
File src/mips/macro-assembler-mips.cc (right):

https://codereview.chromium.org/1396133002/diff/180001/src/mips/macro-assembl...
src/mips/macro-assembler-mips.cc:2504: offset = GetOffset(offset, L, 16);
On 2015/10/15 11:49:08, ivica.bogosavljevic wrote:
> 16 -> constant. it applies elsewhere as well

Done.

https://codereview.chromium.org/1396133002/diff/180001/src/mips/macro-assembl...
src/mips/macro-assembler-mips.cc:5705: // would be tricky.
On 2015/10/15 01:51:24, paul.l... wrote:
> This comment block is a little out of date. Same basic idea, but you might add
> something about tricky encoding of r6 compact branches makes this simple idea
> have a non-trivial implementation.

Done.

https://codereview.chromium.org/1396133002/diff/180001/src/mips/macro-assembl...
src/mips/macro-assembler-mips.cc:5717: opcode = POP30;
On 2015/10/15 01:51:24, paul.l... wrote:
> Even with the comments above, I really don't like all this POP junk. Maybe
just
> add the comments locally for clarity:
> 
>   opecode = POP30;  // change BEQC to BNEC.

Done.

https://codereview.chromium.org/1396133002/diff/180001/src/mips/simulator-mip...
File src/mips/simulator-mips.cc (right):

https://codereview.chromium.org/1396133002/diff/180001/src/mips/simulator-mip...
src/mips/simulator-mips.cc:3872: case POP06:
On 2015/10/15 01:51:24, paul.l... wrote:
> even though the actual instruction names are just below, I wonder if it would
> make these _terrible_ opcode names easier to read if you just add the list of
> supported instructions as a comment:
> 
> case POP06:  // BLEZALC, BGEUC, BLEZ (r2)
> 
> Or do you think since POP06 is an alias of BLEZ, it might make sense to do
this:
> 
> case BLEZ:  //  includes mips32r6 BLEZALC, BGEUC
> 
> Maybe mention the funny-name opcode for cross-reference
> 
> case BLEZ:  // mips32r6 BLEZALC, BGEUC (opcode group POP06)
> 
> Let's discuss this one more .....

Done.

https://codereview.chromium.org/1396133002/diff/180001/src/mips/simulator-mips.h
File src/mips/simulator-mips.h (right):

https://codereview.chromium.org/1396133002/diff/180001/src/mips/simulator-mip...
src/mips/simulator-mips.h:352: Instruction* branch_delay_instr =
On 2015/10/15 01:51:24, paul.l... wrote:
> nit: Improper use of 'delay' in name: this should be instr_after_branch or
> instr_aftter_compact_branch, or inst_after_cb.

Done.

[paul.l..., 2015-11-03 20:01:59]

LGTM!

[balazs.kilvady, 2015-11-03 20:03:12]

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

[commit-bot: I haz the power, 2015-11-03 20:03:25]

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

[commit-bot: I haz the power, 2015-11-03 20:27:24]

Committed patchset #16 (id:340001)

[commit-bot: I haz the power, 2015-11-03 20:27:39]

Patchset 16 (id:??) landed as
https://crrev.com/3573d3cb58bffb08d0537eaa3803fb5c2a5e5639
Cr-Commit-Position: refs/heads/master@{#31761}