MIPS: Use JIC/JIALC offset when possible

MIPS: Use JIC/JIALC offset when possible

Until now JIC and JIALC compact branches were emited without using their
offset. Here we optimize their use by using offset after addition and/or
load immediate operations.

The CL also fixes a problem with deserialization that occurs when a code
object ends with an optimized LUI/AUI and JIC/JIALC instruction pair.
Deserializer processed these instruction pairs by moving to a location
immediately after it, but when this location is the end of the object it
would finish with the current object before doing relocation. This is
fixed by moving the deserializer one instruction before the location of
the instruction pair end.

BUG=

Review-Url: https://codereview.chromium.org/2542403002
Cr-Commit-Position: refs/heads/master@{#44841}
Committed: https://chromium.googlesource.com/v8/v8/+/fe916cd224a5bf8cbf1e8503a192c27fdbcda9f7

[miran.karic, 2016-12-02 08:16:49]

miran.karic@imgtec.com changed reviewers:
+ dusan.simicic@imgtec.com, ilija.pavlovic@imgtec.com,
ivica.bogosavljevic@imgtec.com, marija.antic@imgtec.com,
predrag.rudic@imgtec.com

[miran.karic, 2016-12-02 08:16:50]

PTAL.

[ivica.bogosavljevic, 2016-12-05 10:48:48]

https://codereview.chromium.org/2542403002/diff/1/src/builtins/mips/builtins-...
File src/builtins/mips/builtins-mips.cc (right):

https://codereview.chromium.org/2542403002/diff/1/src/builtins/mips/builtins-...
src/builtins/mips/builtins-mips.cc:532: __ Jump(v0, Code::kHeaderSize -
kHeapObjectTag);
Common routine

https://codereview.chromium.org/2542403002/diff/1/src/compiler/mips/code-gene...
File src/compiler/mips/code-generator-mips.cc (right):

https://codereview.chromium.org/2542403002/diff/1/src/compiler/mips/code-gene...
src/compiler/mips/code-generator-mips.cc:623: if (IsMipsArchVariant(kMips32r6))
{
Move to a common routine

https://codereview.chromium.org/2542403002/diff/1/src/mips/code-stubs-mips.cc
File src/mips/code-stubs-mips.cc (right):

https://codereview.chromium.org/2542403002/diff/1/src/mips/code-stubs-mips.cc...
src/mips/code-stubs-mips.cc:1119: __ Addu(t9, a1, a2);
Can we move these common fragments of code with if (IsMipsArchVariant)... to
macro assembler in order to avoid different
code paths for different architectures?

https://codereview.chromium.org/2542403002/diff/1/src/mips/code-stubs-mips.cc...
src/mips/code-stubs-mips.cc:1259: __ Call(t9, Code::kHeaderSize -
kHeapObjectTag);
Same as above

https://codereview.chromium.org/2542403002/diff/1/src/mips/code-stubs-mips.cc...
src/mips/code-stubs-mips.cc:2787: if (!IsMipsArchVariant(kMips32r6)) {
same as above

https://codereview.chromium.org/2542403002/diff/1/src/mips/macro-assembler-mi...
File src/mips/macro-assembler-mips.cc (right):

https://codereview.chromium.org/2542403002/diff/1/src/mips/macro-assembler-mi...
src/mips/macro-assembler-mips.cc:3682: if (cond == cc_always) {
What happens in this branch if offset != 0. We should add additional
instructions in case offset != 0

https://codereview.chromium.org/2542403002/diff/1/src/mips/macro-assembler-mi...
src/mips/macro-assembler-mips.cc:3711: RecordRelocInfo(rmode, target);
Why are we recording reloc info for mipsr6 but not for mipsr2?

https://codereview.chromium.org/2542403002/diff/1/src/mips/macro-assembler-mi...
src/mips/macro-assembler-mips.cc:3785: jalr(target);
samo as for Jump

https://codereview.chromium.org/2542403002/diff/1/src/mips/macro-assembler-mi...
src/mips/macro-assembler-mips.cc:3830: RecordRelocInfo(rmode, target_int);
same as for :Call from above

[miran.karic, 2016-12-05 12:44:48]

https://codereview.chromium.org/2542403002/diff/1/src/mips/code-stubs-mips.cc
File src/mips/code-stubs-mips.cc (right):

https://codereview.chromium.org/2542403002/diff/1/src/mips/code-stubs-mips.cc...
src/mips/code-stubs-mips.cc:1119: __ Addu(t9, a1, a2);
On 2016/12/05 10:48:47, ivica.bogosavljevic wrote:
> Can we move these common fragments of code with if (IsMipsArchVariant)... to
> macro assembler in order to avoid different
> code paths for different architectures?

Yes, as I commented in MacroAssembler, perhaps I can just use Jump/Call with
offset in any case and then inside these functions use Jump/Call with offset or
add Addu or whatever is needed depending on input type, properties and
architecture.

https://codereview.chromium.org/2542403002/diff/1/src/mips/macro-assembler-mi...
File src/mips/macro-assembler-mips.cc (right):

https://codereview.chromium.org/2542403002/diff/1/src/mips/macro-assembler-mi...
src/mips/macro-assembler-mips.cc:3682: if (cond == cc_always) {
On 2016/12/05 10:48:48, ivica.bogosavljevic wrote:
> What happens in this branch if offset != 0. We should add additional
> instructions in case offset != 0

Currently offset is used only for r6, so here it is always 0. Perhaps though,
maybe it would make sense to use Jump/Call with offset to set the address
instead of doing it before Jump/Call. This would place add/li instructions
inside Jump/Call and we would check architecture only here.

https://codereview.chromium.org/2542403002/diff/1/src/mips/macro-assembler-mi...
src/mips/macro-assembler-mips.cc:3711: RecordRelocInfo(rmode, target);
On 2016/12/05 10:48:48, ivica.bogosavljevic wrote:
> Why are we recording reloc info for mipsr6 but not for mipsr2?

We do it in li() for lui, ori, and here for lui, jic.

[miran.karic, 2016-12-08 11:47:59]

Uploaded a new patch set, PTAL.

[ivica.bogosavljevic, 2016-12-12 13:29:40]

lgtm with nits

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

https://codereview.chromium.org/2542403002/diff/20001/src/mips/assembler-mips...
src/mips/assembler-mips.cc:3205: // Assemble the 32 bit value.
You already checked IsLui in the previous if statement, and you are calling the
same function again.

make two nested if's or save the value of IsLui to a variable in order to avoid
calling it two times.

https://codereview.chromium.org/2542403002/diff/20001/src/mips/macro-assemble...
File src/mips/macro-assembler-mips.h (right):

https://codereview.chromium.org/2542403002/diff/20001/src/mips/macro-assemble...
src/mips/macro-assembler-mips.h:1754: void CallCFunctionHelper(Register
function, int num_reg_arguments,
the function is now a misleading name, a better name would be base,

also base and offset parameters should be probably one next to the other,
because they are a logical whole

[miran.karic, 2017-04-24 13:00:33]

Description was changed from

==========
MIPS: Use JIC/JIALC offset when possible

Until now JIC and JIALC compact branches were emited without using their
offset. Here we optimize their use by using offset after addition and/or
load immediate operations.

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

to

==========
MIPS: Use JIC/JIALC offset when possible

Until now JIC and JIALC compact branches were emited without using their
offset. Here we optimize their use by using offset after addition and/or
load immediate operations.

The CL also fixes a problem with deserialization that occurs when a code
    object ends with an optimized LUI/AUI and JIC/JIALC instruction pair.
    Deserializer processed these instruction pairs by moving to a location
    immediately after it, but when this location is the end of the object it
    would finish with the current object before doing relocation. This is
    fixed by moving the deserializer one instruction before the instruction
    pair end location.

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

[miran.karic, 2017-04-24 13:01:53]

Description was changed from

==========
MIPS: Use JIC/JIALC offset when possible

Until now JIC and JIALC compact branches were emited without using their
offset. Here we optimize their use by using offset after addition and/or
load immediate operations.

The CL also fixes a problem with deserialization that occurs when a code
    object ends with an optimized LUI/AUI and JIC/JIALC instruction pair.
    Deserializer processed these instruction pairs by moving to a location
    immediately after it, but when this location is the end of the object it
    would finish with the current object before doing relocation. This is
    fixed by moving the deserializer one instruction before the instruction
    pair end location.

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

to

==========
MIPS: Use JIC/JIALC offset when possible

Until now JIC and JIALC compact branches were emited without using their
offset. Here we optimize their use by using offset after addition and/or
load immediate operations.

The CL also fixes a problem with deserialization that occurs when a code
object ends with an optimized LUI/AUI and JIC/JIALC instruction pair.
Deserializer processed these instruction pairs by moving to a location
immediately after it, but when this location is the end of the object it
would finish with the current object before doing relocation. This is
fixed by moving the deserializer one instruction before the location of
the instruction pair end.

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

[ivica.bogosavljevic, 2017-04-24 13:42:39]

lgtm

[dusan.simicic, 2017-04-24 14:48:56]

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

https://codereview.chromium.org/2542403002/diff/40001/src/mips/assembler-mips...
src/mips/assembler-mips.h:604: (kInstructionsFor32BitConstant - 1) * kInstrSize,
If you changed this constant expression for R6 in all places in code, maybe it
would be better to redefine kInstructionsFor32BitConstant constant to have one
value for R6 and another for lower architectures.

[miran.karic, 2017-04-25 09:57:47]

On 2017/04/24 14:48:56, dusan.simicic wrote:
>
https://codereview.chromium.org/2542403002/diff/40001/src/mips/assembler-mips.h
> File src/mips/assembler-mips.h (right):
> 
>
https://codereview.chromium.org/2542403002/diff/40001/src/mips/assembler-mips...
> src/mips/assembler-mips.h:604: (kInstructionsFor32BitConstant - 1) *
kInstrSize,
> If you changed this constant expression for R6 in all places in code, maybe it
> would be better to redefine kInstructionsFor32BitConstant constant to have one
> value for R6 and another for lower architectures.

The constant is not changed, here we shift the resulting location by one
instruction.

[miran.karic, 2017-04-25 11:05:07]

The CQ bit was checked by miran.karic@imgtec.com

[commit-bot: I haz the power, 2017-04-25 11:05:19]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dusan.simicic, 2017-04-25 11:24:10]

dusan.simicic@imgtec.com changed reviewers:
+ miran.karic@imgtec.com
- Miran.Karic@imgtec.com

[dusan.simicic, 2017-04-25 11:24:10]

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

https://codereview.chromium.org/2542403002/diff/40001/src/mips/macro-assemble...
src/mips/macro-assembler-mips.cc:3606: DCHECK(is_int16(offset));
'offset' always fits in int16 in this method, DCHECK() is not necessary?

https://codereview.chromium.org/2542403002/diff/40001/src/mips/macro-assemble...
src/mips/macro-assembler-mips.cc:3634: DCHECK(is_int16(offset));
Same comment as above.

https://codereview.chromium.org/2542403002/diff/40001/src/mips/macro-assemble...
src/mips/macro-assembler-mips.cc:3709: // This is not an issue, t9 is expected
to be clobbered anyway.
This comment is related to 'else' block now. Maybe is better to move it down.

[commit-bot: I haz the power, 2017-04-25 11:32:14]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493118307694700,
"parent_rev": "9372dd95d9f9c855e8b7b17e840b6205b60ee328", "commit_rev":
"fe916cd224a5bf8cbf1e8503a192c27fdbcda9f7"}

[commit-bot: I haz the power, 2017-04-25 11:32:19]

Description was changed from

==========
MIPS: Use JIC/JIALC offset when possible

Until now JIC and JIALC compact branches were emited without using their
offset. Here we optimize their use by using offset after addition and/or
load immediate operations.

The CL also fixes a problem with deserialization that occurs when a code
object ends with an optimized LUI/AUI and JIC/JIALC instruction pair.
Deserializer processed these instruction pairs by moving to a location
immediately after it, but when this location is the end of the object it
would finish with the current object before doing relocation. This is
fixed by moving the deserializer one instruction before the location of
the instruction pair end.

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

to

==========
MIPS: Use JIC/JIALC offset when possible

Until now JIC and JIALC compact branches were emited without using their
offset. Here we optimize their use by using offset after addition and/or
load immediate operations.

The CL also fixes a problem with deserialization that occurs when a code
object ends with an optimized LUI/AUI and JIC/JIALC instruction pair.
Deserializer processed these instruction pairs by moving to a location
immediately after it, but when this location is the end of the object it
would finish with the current object before doing relocation. This is
fixed by moving the deserializer one instruction before the location of
the instruction pair end.

BUG=

Review-Url: https://codereview.chromium.org/2542403002
Cr-Commit-Position: refs/heads/master@{#44841}
Committed:
https://chromium.googlesource.com/v8/v8/+/fe916cd224a5bf8cbf1e8503a192c27fdbc...
==========

[commit-bot: I haz the power, 2017-04-25 11:32:20]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/v8/v8/+/fe916cd224a5bf8cbf1e8503a192c27fdbc...