Issue 1534183002: MIPS64: r6 compact branch optimization.

ivica.bogosavljevic

Description was changed from ========== MIPS64: r6 compact branch optimization. Several ports to enable r6 ...

5 years ago (2015-12-18 08:52:03 UTC) #1

Description was changed from

==========
MIPS64: r6 compact branch optimization.

Several ports to enable r6 compact branch optimizations

Port 3573d3cb58bffb08d0537eaa3803fb5c2a5e5639

Original commit message:
MIPS: r6 compact branch optimization.

Port bddf8c9e081f70fc1dad8343040bb4c97dda8890

Original commit message:
MIPS: Fix trampoline pool handling in MacroAssembler::BranchShort()

Port 6993cd0de5ef7edd437c2e45c958d079f27bdd59

Original commit message:
MIPS: Fix 'MIPS:r6 compact branch optimization.'

Jic and jialc compact branch ops are fixed as they does
not have 'forbidden slot' restriction. Also COP1 branches
(CTI instructions) added to IsForbiddenAfterBranchInstr().

Port bb332195d32f1e6f99e9cd065ef559be9bcca19b

Original commit message:
MIPS: Fix trampoline pool handling in MacroAssembler::BranchShort()

Port c91bcf719208f12a88f6c9fcf6d73204762739f2

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

BUG=
==========

to

==========
MIPS64: r6 compact branch optimization.

Several ports to enable r6 compact branch optimizations on MIPS64

Port 3573d3cb58bffb08d0537eaa3803fb5c2a5e5639

Original commit message:
MIPS: r6 compact branch optimization.

Port bddf8c9e081f70fc1dad8343040bb4c97dda8890

Original commit message:
MIPS: Fix trampoline pool handling in MacroAssembler::BranchShort()

Port 6993cd0de5ef7edd437c2e45c958d079f27bdd59

Original commit message:
MIPS: Fix 'MIPS:r6 compact branch optimization.'

Jic and jialc compact branch ops are fixed as they does
not have 'forbidden slot' restriction. Also COP1 branches
(CTI instructions) added to IsForbiddenAfterBranchInstr().

Port bb332195d32f1e6f99e9cd065ef559be9bcca19b

Original commit message:
MIPS: Fix trampoline pool handling in MacroAssembler::BranchShort()

Port c91bcf719208f12a88f6c9fcf6d73204762739f2

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

BUG=
==========

ivica.bogosavljevic

ivica.bogosavljevic@imgtec.com changed reviewers: + akos.palfi@imgtec.com, balazs.kilvady@imgtec.com, djordje.pesic@imgtec.com, ilija.pavlovic@imgtec.com, miran.karic@imgtec.com, paul.lind@imgtec.com

5 years ago (2015-12-18 08:56:32 UTC) #2

ivica.bogosavljevic

https://codereview.chromium.org/1534183002/diff/1/src/compiler/mips64/code-generator-mips64.cc File src/compiler/mips64/code-generator-mips64.cc (right): https://codereview.chromium.org/1534183002/diff/1/src/compiler/mips64/code-generator-mips64.cc#newcode1556 src/compiler/mips64/code-generator-mips64.cc:1556: Register input = i.InputRegister(0); Patch tested on MIPS64 R2 ...

5 years ago (2015-12-18 08:56:32 UTC) #3

Ilija.Pavlovic1

ilija.pavlovic@imgtec.com changed reviewers: + Ilija.Pavlovic@imgtec.com

5 years ago (2015-12-18 11:38:21 UTC) #4

Ilija.Pavlovic1

https://codereview.chromium.org/1534183002/diff/1/src/mips64/assembler-mips64-inl.h File src/mips64/assembler-mips64-inl.h (right): https://codereview.chromium.org/1534183002/diff/1/src/mips64/assembler-mips64-inl.h#newcode468 src/mips64/assembler-mips64-inl.h:468: // Nop instruction to preceed a CTI in forbidden ...

5 years ago (2015-12-18 11:38:22 UTC) #5

balazs.kilvady

balazs.kilvady@imgtec.com changed reviewers: - Ilija.Pavlovic@imgtec.com

5 years ago (2015-12-18 19:18:05 UTC) #6

balazs.kilvady

Looks good to me but I noticed for the first review, please take a look. ...

5 years ago (2015-12-18 19:18:05 UTC) #7

ivica.bogosavljevic

https://codereview.chromium.org/1534183002/diff/1/src/compiler/mips64/code-generator-mips64.cc File src/compiler/mips64/code-generator-mips64.cc (right): https://codereview.chromium.org/1534183002/diff/1/src/compiler/mips64/code-generator-mips64.cc#newcode1556 src/compiler/mips64/code-generator-mips64.cc:1556: Register input = i.InputRegister(0); On 2015/12/18 08:56:32, ivica.bogosavljevic wrote: ...

5 years ago (2015-12-22 10:22:40 UTC) #8

https://codereview.chromium.org/1534183002/diff/1/src/compiler/mips64/code-ge...
File src/compiler/mips64/code-generator-mips64.cc (right):

https://codereview.chromium.org/1534183002/diff/1/src/compiler/mips64/code-ge...
src/compiler/mips64/code-generator-mips64.cc:1556: Register input =
i.InputRegister(0);
On 2015/12/18 08:56:32, ivica.bogosavljevic wrote:
> Patch tested on MIPS64 R2 and R6, simulator and qemu-userland and no
regressions
> found

Patch also tested on MIPS64R6 Big-Endian under qemu-userland and no regressions
found

https://codereview.chromium.org/1534183002/diff/1/src/mips64/assembler-mips64...
File src/mips64/assembler-mips64-inl.h (right):

https://codereview.chromium.org/1534183002/diff/1/src/mips64/assembler-mips64...
src/mips64/assembler-mips64-inl.h:468: // Nop instruction to preceed a CTI in
forbidden slot:
On 2015/12/18 19:18:05, balazs.kilvady wrote:
> On 2015/12/18 11:38:22, Ilija.Pavlovic1 wrote:
> > Missing check IsForbiddenAfterBranchInstr(x)?
> 
> Also when we emitting a 64-bit value that is not an instruction (we are
emitting
> 64-bit addresses this way) I think, and that cannot be a
> `IsForbiddenAfterBranchInstr(x)` but the upper 32 bits of a 64-bit value might
> look like a branch istruction for the CPU. (For example a if the upper 32 bits
> holds 8 that "looks like" a jr, as I remember. So the most significant 32 bits
> of 'x' must be passed to IsForbiddenAfterBranchInstr() and also please take
care
> of little/big endian coding.

There is no need for IsForbiddenAfterBranchInstr(x) here because x is not an
instruction. We need to emit a NOP here unconditionally, and these are the
reasons: 
1) I believe that the instruction in the forbidden slot enters the decoding
phase in the CPU pipeline, this could create a condition of ReservedInstruction
exception even though the instruction itself doesn't get executed. (according to
the instruction manual this should not happen, but there will be many
architectures where this could actually happen)
2) The saving we get if we add this is not big. Normally there is one jump, and
a list of constants emitted one after another.
3) Additional complexity because we need to make a difference between big endian
and little endian encodings.

https://codereview.chromium.org/1534183002/diff/1/src/mips64/assembler-mips64...
src/mips64/assembler-mips64-inl.h:476: pc_ += kInstrSize * 2;
On 2015/12/18 11:38:22, Ilija.Pavlovic1 wrote:
> Missing check is_compact_branch == COMPACT_BRANCH ? (similar to lines 456 -
458)

Done.

https://codereview.chromium.org/1534183002/diff/1/src/mips64/assembler-mips64...
src/mips64/assembler-mips64-inl.h:476: pc_ += kInstrSize * 2;
On 2015/12/18 19:18:05, balazs.kilvady wrote:
> On 2015/12/18 11:38:22, Ilija.Pavlovic1 wrote:
> > Missing check is_compact_branch == COMPACT_BRANCH ? (similar to lines 456 -
> 458)
> 
> A 64-bit value cannot be a real instruction so it cannot be a compact branch,
I
> think.

Acknowledged.

https://codereview.chromium.org/1534183002/diff/1/src/mips64/assembler-mips64.cc
File src/mips64/assembler-mips64.cc (right):

https://codereview.chromium.org/1534183002/diff/1/src/mips64/assembler-mips64...
src/mips64/assembler-mips64.cc:1427: GenInstrImmediate(BGTZ, zero_reg, rt,
offset,
On 2015/12/18 11:38:22, Ilija.Pavlovic1 wrote:
> Before GenInstrImmediate, to add
> "positions_recorder()->WriteRecordedPositions();" ?

Acknowledged.

https://codereview.chromium.org/1534183002/diff/1/src/mips64/assembler-mips64...
src/mips64/assembler-mips64.cc:1445:
positions_recorder()->WriteRecordedPositions();
On 2015/12/18 19:18:05, balazs.kilvady wrote:
> one call of 'positions_recorder()->WriteRecordedPositions();' must be enough
:)

Acknowledged.

https://codereview.chromium.org/1534183002/diff/1/src/mips64/assembler-mips64.h
File src/mips64/assembler-mips64.h (right):

https://codereview.chromium.org/1534183002/diff/1/src/mips64/assembler-mips64...
src/mips64/assembler-mips64.h:1270: void EmitPendingInstructions() {
On 2015/12/18 19:18:05, balazs.kilvady wrote:
> Nice solution. Should be backported to 32-bit. Although I think a better
> function name could be used. This function emits only a branch-safe nop if
> needed not any "pending" instruction. What about
EmitCompactBranchInstruction()
> to match to other functions from this function group?

I was thinking the same thing as you did, to use the name
EmitCompactBranchForbiddenSlotInstruction, but the reason why i though this name
a bit better is basically with this change we introduce some kind of delayed
assembly. Delayed assembly allows us to wait until a sequence of instructions is
generated and do some optimizations on it. So if it is ok, I would like to keep
this name

https://codereview.chromium.org/1534183002/diff/1/src/mips64/assembler-mips64...
src/mips64/assembler-mips64.h:1328: 
On 2015/12/18 19:18:05, balazs.kilvady wrote:
> A 64-bit value cannot be a real instruction so it cannot be a compact branch,
I
> think. If it is true then please remove the 2nd parameter of this
> emit(uint64_t).

Acknowledged.

https://codereview.chromium.org/1534183002/diff/1/src/mips64/constants-mips64.cc
File src/mips64/constants-mips64.cc (right):

https://codereview.chromium.org/1534183002/diff/1/src/mips64/constants-mips64...
src/mips64/constants-mips64.cc:145: case POP30:  // bnezalc, bvnc, bnec
On 2015/12/18 19:18:05, balazs.kilvady wrote:
> I mistyped again, please: s/bvnc/bnvc

Acknowledged.

https://codereview.chromium.org/1534183002/diff/1/src/mips64/constants-mips64.h
File src/mips64/constants-mips64.h (right):

https://codereview.chromium.org/1534183002/diff/1/src/mips64/constants-mips64...
src/mips64/constants-mips64.h:388: POP30 = DADDI,  // bnezalc, bvnc, bnec
On 2015/12/18 11:38:22, Ilija.Pavlovic1 wrote:
> change: "bvnc" into "bnvc"

Acknowledged.

https://codereview.chromium.org/1534183002/diff/1/src/mips64/constants-mips64...
src/mips64/constants-mips64.h:388: POP30 = DADDI,  // bnezalc, bvnc, bnec
On 2015/12/18 19:18:05, balazs.kilvady wrote:
> On 2015/12/18 11:38:22, Ilija.Pavlovic1 wrote:
> > change: "bvnc" into "bnvc"
> Mea culpa, That is a typo from the 32-bit version. One more thing to add to a
> followup CL :)

Acknowledged.

https://codereview.chromium.org/1534183002/diff/1/src/mips64/disasm-mips64.cc
File src/mips64/disasm-mips64.cc (right):

https://codereview.chromium.org/1534183002/diff/1/src/mips64/disasm-mips64.cc...
src/mips64/disasm-mips64.cc:1695: } else {
On 2015/12/18 11:38:22, Ilija.Pavlovic1 wrote:
> One of conditions is (rt_reg != 0):
> 
> if (rt_reg != 0) {
>     if (rs_reg == 0) { ... }
>     else { ... }
> } else { // rt_reg == 0
>     UNREACHABLE(); 
> }

I'll add DCHECK(rt_reg > 0) before if (rs_reg == 0)

https://codereview.chromium.org/1534183002/diff/1/src/mips64/disasm-mips64.cc...
src/mips64/disasm-mips64.cc:1695: } else {
On 2015/12/18 19:18:05, balazs.kilvady wrote:
> On 2015/12/18 11:38:22, Ilija.Pavlovic1 wrote:
> > One of conditions is (rt_reg != 0):
> > 
> > if (rt_reg != 0) {
> >     if (rs_reg == 0) { ... }
> >     else { ... }
> > } else { // rt_reg == 0
> >     UNREACHABLE(); 
> > }
> 
> One more thing to backport...

Acknowledged.

https://codereview.chromium.org/1534183002/diff/1/src/mips64/disasm-mips64.cc...
src/mips64/disasm-mips64.cc:1713: } else {
On 2015/12/18 11:38:22, Ilija.Pavlovic1 wrote:
> One of conditions is (rt_reg != 0):
> 
> if (rt_reg != 0) {
>     if (rs_reg == 0) { ... }
>     else { ... }
> } else { // rt_reg == 0
>     UNREACHABLE(); 
> }

I'll add DCHECK(rt_reg > 0) before if (rs_reg == 0)

https://codereview.chromium.org/1534183002/diff/1/src/mips64/simulator-mips64.cc
File src/mips64/simulator-mips64.cc (right):

https://codereview.chromium.org/1534183002/diff/1/src/mips64/simulator-mips64...
src/mips64/simulator-mips64.cc:149: #define UNSUPPORTED() printf("Sim:
Unsupported instruction.\n");
On 2015/12/18 19:18:05, balazs.kilvady wrote:
> I just forgot to remove it from the final CL but I think it is useful in
> debugging as Instruction prints the same without 'Sim: ' :)

Ok, I'll remove Sim here as well as for MIPS32

https://codereview.chromium.org/1534183002/diff/1/src/mips64/simulator-mips64.h
File src/mips64/simulator-mips64.h (right):

https://codereview.chromium.org/1534183002/diff/1/src/mips64/simulator-mips64...
src/mips64/simulator-mips64.h:399: V8_Fatal(__FILE__, __LINE__,
On 2015/12/18 19:18:05, balazs.kilvady wrote:
> I think we should change the function name from 'IsForbiddenInBranchDelay' to
> 'IsForbiddenAfterBranch' on both 32 and 64 bits. What do you think?

Agree. Maybe we should have it IsForbiddenAfterBranchCompact if we are
performing the check on BranchCompact instructions.

balazs.kilvady

https://codereview.chromium.org/1534183002/diff/1/src/mips64/assembler-mips64-inl.h File src/mips64/assembler-mips64-inl.h (right): https://codereview.chromium.org/1534183002/diff/1/src/mips64/assembler-mips64-inl.h#newcode468 src/mips64/assembler-mips64-inl.h:468: // Nop instruction to preceed a CTI in forbidden ...

5 years ago (2015-12-22 11:01:56 UTC) #9

https://codereview.chromium.org/1534183002/diff/1/src/mips64/assembler-mips64...
File src/mips64/assembler-mips64-inl.h (right):

https://codereview.chromium.org/1534183002/diff/1/src/mips64/assembler-mips64...
src/mips64/assembler-mips64-inl.h:468: // Nop instruction to preceed a CTI in
forbidden slot:
On 2015/12/22 10:22:39, ivica.bogosavljevic wrote:
> On 2015/12/18 19:18:05, balazs.kilvady wrote:
> > On 2015/12/18 11:38:22, Ilija.Pavlovic1 wrote:
> > > Missing check IsForbiddenAfterBranchInstr(x)?
> > 
> > Also when we emitting a 64-bit value that is not an instruction (we are
> emitting
> > 64-bit addresses this way) I think, and that cannot be a
> > `IsForbiddenAfterBranchInstr(x)` but the upper 32 bits of a 64-bit value
might
> > look like a branch istruction for the CPU. (For example a if the upper 32
bits
> > holds 8 that "looks like" a jr, as I remember. So the most significant 32
bits
> > of 'x' must be passed to IsForbiddenAfterBranchInstr() and also please take
> care
> > of little/big endian coding.
> 
> There is no need for IsForbiddenAfterBranchInstr(x) here because x is not an
> instruction. We need to emit a NOP here unconditionally, and these are the
> reasons: 
> 1) I believe that the instruction in the forbidden slot enters the decoding
> phase in the CPU pipeline, this could create a condition of
ReservedInstruction
> exception even though the instruction itself doesn't get executed. (according
to
> the instruction manual this should not happen, but there will be many
> architectures where this could actually happen)
I guess in this case the CPU could not know if the next 32 bits holds something
which won't be executed ever.

> 2) The saving we get if we add this is not big. Normally there is one jump,
and
> a list of constants emitted one after another.
> 3) Additional complexity because we need to make a difference between big
endian
> and little endian encodings.
I think the endianess is #define-d so a
#ifdef LITTLEENDIAN
Instruction::IsForbiddenAfterBranchInstr(static_cast<Instr>(x));
#else
Instruction::IsForbiddenAfterBranchInstr(static_cast<Instr>(x >> 32));
#endif
is not a big deal. I would vote to add something like this. At least worth a
test if our assumptation is right. Otherwise we just put safety nop()s
everywhere.

https://codereview.chromium.org/1534183002/diff/1/src/mips64/assembler-mips64.h
File src/mips64/assembler-mips64.h (right):

https://codereview.chromium.org/1534183002/diff/1/src/mips64/assembler-mips64...
src/mips64/assembler-mips64.h:1270: void EmitPendingInstructions() {
On 2015/12/22 10:22:39, ivica.bogosavljevic wrote:
> On 2015/12/18 19:18:05, balazs.kilvady wrote:
> > Nice solution. Should be backported to 32-bit. Although I think a better
> > function name could be used. This function emits only a branch-safe nop if
> > needed not any "pending" instruction. What about
> EmitCompactBranchInstruction()
> > to match to other functions from this function group?
> 
> I was thinking the same thing as you did, to use the name
> EmitCompactBranchForbiddenSlotInstruction, but the reason why i though this
name
> a bit better is basically with this change we introduce some kind of delayed
> assembly. Delayed assembly allows us to wait until a sequence of instructions
is
> generated and do some optimizations on it. So if it is ok, I would like to
keep
> this name

I don't agree. Actually Assembler class is for direct MIPS assembly instructions
and MacroAssembler is for adding some 'tricks' to the assembler. Actually this
forbidden-slot-guard is breaking the rule which is not nice. But we should not
add any more delay tricks to Assembler class IMO. So we should not use this
function for any other purposes and we should not add a generic name to it.

https://codereview.chromium.org/1534183002/diff/1/src/mips64/simulator-mips64.cc
File src/mips64/simulator-mips64.cc (right):

https://codereview.chromium.org/1534183002/diff/1/src/mips64/simulator-mips64...
src/mips64/simulator-mips64.cc:149: #define UNSUPPORTED() printf("Sim:
Unsupported instruction.\n");
On 2015/12/22 10:22:40, ivica.bogosavljevic wrote:
> On 2015/12/18 19:18:05, balazs.kilvady wrote:
> > I just forgot to remove it from the final CL but I think it is useful in
> > debugging as Instruction prints the same without 'Sim: ' :)
> 
> Ok, I'll remove Sim here as well as for MIPS32
Please do not remove it. I think it is fine to make difference between
constant.cc:Instruction and simulator error messages. I had to add it many times
for debugging. If it has passed reviews on 32 bits then let's keep it.

https://codereview.chromium.org/1534183002/diff/1/src/mips64/simulator-mips64.h
File src/mips64/simulator-mips64.h (right):

https://codereview.chromium.org/1534183002/diff/1/src/mips64/simulator-mips64...
src/mips64/simulator-mips64.h:399: V8_Fatal(__FILE__, __LINE__,
On 2015/12/22 10:22:40, ivica.bogosavljevic wrote:
> On 2015/12/18 19:18:05, balazs.kilvady wrote:
> > I think we should change the function name from 'IsForbiddenInBranchDelay'
to
> > 'IsForbiddenAfterBranch' on both 32 and 64 bits. What do you think?
> 
> Agree. Maybe we should have it IsForbiddenAfterBranchCompact if we are
> performing the check on BranchCompact instructions.
We can use the same function for r6 compact branches and for pre-r6 and r6 delay
slots so the 'Compact' word should not be added.

ivica.bogosavljevic

https://codereview.chromium.org/1534183002/diff/1/src/mips64/assembler-mips64.h File src/mips64/assembler-mips64.h (right): https://codereview.chromium.org/1534183002/diff/1/src/mips64/assembler-mips64.h#newcode1270 src/mips64/assembler-mips64.h:1270: void EmitPendingInstructions() { On 2015/12/22 11:01:56, balazs.kilvady wrote: > ...

5 years ago (2015-12-22 12:23:42 UTC) #10

https://codereview.chromium.org/1534183002/diff/1/src/mips64/assembler-mips64.h
File src/mips64/assembler-mips64.h (right):

https://codereview.chromium.org/1534183002/diff/1/src/mips64/assembler-mips64...
src/mips64/assembler-mips64.h:1270: void EmitPendingInstructions() {
On 2015/12/22 11:01:56, balazs.kilvady wrote:
> On 2015/12/22 10:22:39, ivica.bogosavljevic wrote:
> > On 2015/12/18 19:18:05, balazs.kilvady wrote:
> > > Nice solution. Should be backported to 32-bit. Although I think a better
> > > function name could be used. This function emits only a branch-safe nop if
> > > needed not any "pending" instruction. What about
> > EmitCompactBranchInstruction()
> > > to match to other functions from this function group?
> > 
> > I was thinking the same thing as you did, to use the name
> > EmitCompactBranchForbiddenSlotInstruction, but the reason why i though this
> name
> > a bit better is basically with this change we introduce some kind of delayed
> > assembly. Delayed assembly allows us to wait until a sequence of
instructions
> is
> > generated and do some optimizations on it. So if it is ok, I would like to
> keep
> > this name
> 
> I don't agree. Actually Assembler class is for direct MIPS assembly
instructions
> and MacroAssembler is for adding some 'tricks' to the assembler. Actually this
> forbidden-slot-guard is breaking the rule which is not nice. But we should not
> add any more delay tricks to Assembler class IMO. So we should not use this
> function for any other purposes and we should not add a generic name to it.
> 

Acknowledged.

https://codereview.chromium.org/1534183002/diff/1/src/mips64/simulator-mips64.cc
File src/mips64/simulator-mips64.cc (right):

https://codereview.chromium.org/1534183002/diff/1/src/mips64/simulator-mips64...
src/mips64/simulator-mips64.cc:149: #define UNSUPPORTED() printf("Sim:
Unsupported instruction.\n");
On 2015/12/22 11:01:56, balazs.kilvady wrote:
> On 2015/12/22 10:22:40, ivica.bogosavljevic wrote:
> > On 2015/12/18 19:18:05, balazs.kilvady wrote:
> > > I just forgot to remove it from the final CL but I think it is useful in
> > > debugging as Instruction prints the same without 'Sim: ' :)
> > 
> > Ok, I'll remove Sim here as well as for MIPS32
> Please do not remove it. I think it is fine to make difference between
> constant.cc:Instruction and simulator error messages. I had to add it many
times
> for debugging. If it has passed reviews on 32 bits then let's keep it.

Acknowledged.

https://codereview.chromium.org/1534183002/diff/1/src/mips64/simulator-mips64.h
File src/mips64/simulator-mips64.h (right):

https://codereview.chromium.org/1534183002/diff/1/src/mips64/simulator-mips64...
src/mips64/simulator-mips64.h:399: V8_Fatal(__FILE__, __LINE__,
On 2015/12/22 11:01:56, balazs.kilvady wrote:
> On 2015/12/22 10:22:40, ivica.bogosavljevic wrote:
> > On 2015/12/18 19:18:05, balazs.kilvady wrote:
> > > I think we should change the function name from 'IsForbiddenInBranchDelay'
> to
> > > 'IsForbiddenAfterBranch' on both 32 and 64 bits. What do you think?
> > 
> > Agree. Maybe we should have it IsForbiddenAfterBranchCompact if we are
> > performing the check on BranchCompact instructions.
> We can use the same function for r6 compact branches and for pre-r6 and r6
delay
> slots so the 'Compact' word should not be added.

Acknowledged.

ivica.bogosavljevic

The CQ bit was checked by ivica.bogosavljevic@imgtec.com to run a CQ dry run

5 years ago (2015-12-23 16:24:59 UTC) #11

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1534183002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1534183002/20001

5 years ago (2015-12-23 16:25:02 UTC) #12

commit-bot: I haz the power

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

5 years ago (2015-12-23 16:25:04 UTC) #13

commit-bot: I haz the power

Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even ...

5 years ago (2015-12-23 16:25:04 UTC) #14

balazs.kilvady

https://codereview.chromium.org/1534183002/diff/1/src/mips64/assembler-mips64-inl.h File src/mips64/assembler-mips64-inl.h (right): https://codereview.chromium.org/1534183002/diff/1/src/mips64/assembler-mips64-inl.h#newcode468 src/mips64/assembler-mips64-inl.h:468: // Nop instruction to preceed a CTI in forbidden ...

4 years, 11 months ago (2016-01-04 13:38:39 UTC) #15

https://codereview.chromium.org/1534183002/diff/1/src/mips64/assembler-mips64...
File src/mips64/assembler-mips64-inl.h (right):

https://codereview.chromium.org/1534183002/diff/1/src/mips64/assembler-mips64...
src/mips64/assembler-mips64-inl.h:468: // Nop instruction to preceed a CTI in
forbidden slot:
On 2015/12/22 11:01:55, balazs.kilvady wrote:
> On 2015/12/22 10:22:39, ivica.bogosavljevic wrote:
> > On 2015/12/18 19:18:05, balazs.kilvady wrote:
> > > On 2015/12/18 11:38:22, Ilija.Pavlovic1 wrote:
> > > > Missing check IsForbiddenAfterBranchInstr(x)?
> > > 
> > > Also when we emitting a 64-bit value that is not an instruction (we are
> > emitting
> > > 64-bit addresses this way) I think, and that cannot be a
> > > `IsForbiddenAfterBranchInstr(x)` but the upper 32 bits of a 64-bit value
> might
> > > look like a branch istruction for the CPU. (For example a if the upper 32
> bits
> > > holds 8 that "looks like" a jr, as I remember. So the most significant 32
> bits
> > > of 'x' must be passed to IsForbiddenAfterBranchInstr() and also please
take
> > care
> > > of little/big endian coding.
> > 
> > There is no need for IsForbiddenAfterBranchInstr(x) here because x is not an
> > instruction. We need to emit a NOP here unconditionally, and these are the
> > reasons: 
> > 1) I believe that the instruction in the forbidden slot enters the decoding
> > phase in the CPU pipeline, this could create a condition of
> ReservedInstruction
> > exception even though the instruction itself doesn't get executed.
(according
> to
> > the instruction manual this should not happen, but there will be many
> > architectures where this could actually happen)
> I guess in this case the CPU could not know if the next 32 bits holds
something
> which won't be executed ever.
> 
> > 2) The saving we get if we add this is not big. Normally there is one jump,
> and
> > a list of constants emitted one after another.
> > 3) Additional complexity because we need to make a difference between big
> endian
> > and little endian encodings.
> I think the endianess is #define-d so a
> #ifdef LITTLEENDIAN
> Instruction::IsForbiddenAfterBranchInstr(static_cast<Instr>(x));
> #else
> Instruction::IsForbiddenAfterBranchInstr(static_cast<Instr>(x >> 32));
> #endif
> is not a big deal. I would vote to add something like this. At least worth a
> test if our assumptation is right. Otherwise we just put safety nop()s
> everywhere.
 I still miss  Instruction::IsForbiddenAfterBranchInstr(static_cast<Instr>([x |
x >> 32])); or at least a comment. I try to modify TEST(jump_tables4) in
test-macro-assembler-mip64.cc to test "corner cases".

balazs.kilvady

https://codereview.chromium.org/1534183002/diff/20001/src/mips64/simulator-mips64.h File src/mips64/simulator-mips64.h (right): https://codereview.chromium.org/1534183002/diff/20001/src/mips64/simulator-mips64.h#newcode396 src/mips64/simulator-mips64.h:396: Instruction* instr_aftter_compact_branch = instr_after_compact_branch instead of instr_aftter_compact_branch (after is ...

4 years, 11 months ago (2016-01-04 15:26:23 UTC) #16

balazs.kilvady

https://codereview.chromium.org/1534183002/diff/20001/src/mips64/simulator-mips64.cc File src/mips64/simulator-mips64.cc (right): https://codereview.chromium.org/1534183002/diff/20001/src/mips64/simulator-mips64.cc#newcode4137 src/mips64/simulator-mips64.cc:4137: }; Add a newline, please. https://codereview.chromium.org/1534183002/diff/20001/src/mips64/simulator-mips64.cc#newcode4272 src/mips64/simulator-mips64.cc:4272: CheckForbiddenSlot(get_pc()); Remove ...

4 years, 11 months ago (2016-01-04 15:30:38 UTC) #17

ivica.bogosavljevic

The CQ bit was checked by ivica.bogosavljevic@imgtec.com to run a CQ dry run

4 years, 11 months ago (2016-01-06 12:58:22 UTC) #19

commit-bot: I haz the power

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

4 years, 11 months ago (2016-01-06 12:58:35 UTC) #20

commit-bot: I haz the power

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

4 years, 11 months ago (2016-01-06 13:30:40 UTC) #21

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 11 months ago (2016-01-06 13:30:41 UTC) #22

ivica.bogosavljevic

The CQ bit was checked by ivica.bogosavljevic@imgtec.com

4 years, 11 months ago (2016-01-06 13:35:01 UTC) #23

ivica.bogosavljevic

The patchset sent to the CQ was uploaded after l-g-t-m from balazs.kilvady@imgtec.com Link to the ...

4 years, 11 months ago (2016-01-06 13:35:01 UTC) #24

commit-bot: I haz the power

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

4 years, 11 months ago (2016-01-06 13:35:12 UTC) #25

commit-bot: I haz the power

Description was changed from ========== MIPS64: r6 compact branch optimization. Several ports to enable r6 ...

4 years, 11 months ago (2016-01-06 13:36:35 UTC) #26

Message was sent while issue was closed.

Description was changed from

==========
MIPS64: r6 compact branch optimization.

Several ports to enable r6 compact branch optimizations on MIPS64

Port 3573d3cb58bffb08d0537eaa3803fb5c2a5e5639

Original commit message:
MIPS: r6 compact branch optimization.

Port bddf8c9e081f70fc1dad8343040bb4c97dda8890

Original commit message:
MIPS: Fix trampoline pool handling in MacroAssembler::BranchShort()

Port 6993cd0de5ef7edd437c2e45c958d079f27bdd59

Original commit message:
MIPS: Fix 'MIPS:r6 compact branch optimization.'

Jic and jialc compact branch ops are fixed as they does
not have 'forbidden slot' restriction. Also COP1 branches
(CTI instructions) added to IsForbiddenAfterBranchInstr().

Port bb332195d32f1e6f99e9cd065ef559be9bcca19b

Original commit message:
MIPS: Fix trampoline pool handling in MacroAssembler::BranchShort()

Port c91bcf719208f12a88f6c9fcf6d73204762739f2

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

BUG=
==========

to

==========
MIPS64: r6 compact branch optimization.

Several ports to enable r6 compact branch optimizations on MIPS64

Port 3573d3cb58bffb08d0537eaa3803fb5c2a5e5639

Original commit message:
MIPS: r6 compact branch optimization.

Port bddf8c9e081f70fc1dad8343040bb4c97dda8890

Original commit message:
MIPS: Fix trampoline pool handling in MacroAssembler::BranchShort()

Port 6993cd0de5ef7edd437c2e45c958d079f27bdd59

Original commit message:
MIPS: Fix 'MIPS:r6 compact branch optimization.'

Jic and jialc compact branch ops are fixed as they does
not have 'forbidden slot' restriction. Also COP1 branches
(CTI instructions) added to IsForbiddenAfterBranchInstr().

Port bb332195d32f1e6f99e9cd065ef559be9bcca19b

Original commit message:
MIPS: Fix trampoline pool handling in MacroAssembler::BranchShort()

Port c91bcf719208f12a88f6c9fcf6d73204762739f2

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

BUG=
==========

commit-bot: I haz the power

Description was changed from ========== MIPS64: r6 compact branch optimization. Several ports to enable r6 ...

4 years, 11 months ago (2016-01-06 13:36:47 UTC) #28

Message was sent while issue was closed.

Description was changed from

==========
MIPS64: r6 compact branch optimization.

Several ports to enable r6 compact branch optimizations on MIPS64

Port 3573d3cb58bffb08d0537eaa3803fb5c2a5e5639

Original commit message:
MIPS: r6 compact branch optimization.

Port bddf8c9e081f70fc1dad8343040bb4c97dda8890

Original commit message:
MIPS: Fix trampoline pool handling in MacroAssembler::BranchShort()

Port 6993cd0de5ef7edd437c2e45c958d079f27bdd59

Original commit message:
MIPS: Fix 'MIPS:r6 compact branch optimization.'

Jic and jialc compact branch ops are fixed as they does
not have 'forbidden slot' restriction. Also COP1 branches
(CTI instructions) added to IsForbiddenAfterBranchInstr().

Port bb332195d32f1e6f99e9cd065ef559be9bcca19b

Original commit message:
MIPS: Fix trampoline pool handling in MacroAssembler::BranchShort()

Port c91bcf719208f12a88f6c9fcf6d73204762739f2

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

BUG=
==========

to

==========
MIPS64: r6 compact branch optimization.

Several ports to enable r6 compact branch optimizations on MIPS64

Port 3573d3cb58bffb08d0537eaa3803fb5c2a5e5639

Original commit message:
MIPS: r6 compact branch optimization.

Port bddf8c9e081f70fc1dad8343040bb4c97dda8890

Original commit message:
MIPS: Fix trampoline pool handling in MacroAssembler::BranchShort()

Port 6993cd0de5ef7edd437c2e45c958d079f27bdd59

Original commit message:
MIPS: Fix 'MIPS:r6 compact branch optimization.'

Jic and jialc compact branch ops are fixed as they does
not have 'forbidden slot' restriction. Also COP1 branches
(CTI instructions) added to IsForbiddenAfterBranchInstr().

Port bb332195d32f1e6f99e9cd065ef559be9bcca19b

Original commit message:
MIPS: Fix trampoline pool handling in MacroAssembler::BranchShort()

Port c91bcf719208f12a88f6c9fcf6d73204762739f2

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

BUG=

Committed: https://crrev.com/2c63060f113bd7a7a40e5aeb895984b8328e7824
Cr-Commit-Position: refs/heads/master@{#33136}
==========

commit-bot: I haz the power

4 years, 11 months ago (2016-01-06 13:36:50 UTC) #29

Message was sent while issue was closed.

Patchset 4 (id:??) landed as
https://crrev.com/2c63060f113bd7a7a40e5aeb895984b8328e7824
Cr-Commit-Position: refs/heads/master@{#33136}

Issue 1534183002: MIPS64: r6 compact branch optimization. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Apply remarks from the review #

Patch Set 3 : Implement code review remarks for simulator-mips64 #

Patch Set 4 : Rebasing master to include the new changes #

Messages