MIPS64: Add optimizations to li macro.

MIPS64: Add optimizations to li macro.

A number of improvements in mips64 load immediate macro is added per
suggestions from MIPS ART team. Also fix Subu and Dsubu macro, add a
test for Subu and Dsubu and make minor code adjustments.

BUG=
TEST=cctest/test-assembler-mips/li_macro
     cctest/test-assembler-mips/Subu
     cctest/test-assembler-mips/Dsubu

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

[miran.karic, 2017-05-19 09:05:55]

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

[miran.karic, 2017-05-19 09:05:56]

PTAL. There is already li_macro test defined in the test-assembler-mips64.cc so
please comment on merging it with li test I defined.

[ivica.bogosavljevic, 2017-05-19 12:42:39]

lgtm with nits

https://codereview.chromium.org/2892163002/diff/20001/src/mips64/constants-mi...
File src/mips64/constants-mips64.h (right):

https://codereview.chromium.org/2892163002/diff/20001/src/mips64/constants-mi...
src/mips64/constants-mips64.h:342: const int kHiMaskOf32 = 0xffff << 16;  //
Only to be used with 32-bit values
If you mean by this
HiMaskOffset32 than you should either use full name HiMaskOffset32 because when
I see it like this, I think HiMask of 32 . This applies to all the constants you
renamed.

[miran.karic, 2017-05-19 13:23:53]

https://codereview.chromium.org/2892163002/diff/20001/src/mips64/constants-mi...
File src/mips64/constants-mips64.h (right):

https://codereview.chromium.org/2892163002/diff/20001/src/mips64/constants-mi...
src/mips64/constants-mips64.h:342: const int kHiMaskOf32 = 0xffff << 16;  //
Only to be used with 32-bit values
On 2017/05/19 12:42:39, ivica.bogosavljevic wrote:
> If you mean by this
> HiMaskOffset32 than you should either use full name HiMaskOffset32 because
when
> I see it like this, I think HiMask of 32 . This applies to all the constants
you
> renamed.

It is HiMask of 32, not offset. This is analogous to existing 16MaskOf64 naming
for 64 bit masks.

[Ilija.Pavlovic1, 2017-05-22 06:59:21]

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

[Ilija.Pavlovic1, 2017-05-22 06:59:21]

lgtm

[dusan.simicic, 2017-05-23 12:17:38]

dusan.simicic@imgtec.com changed reviewers:
- Ilija.Pavlovic@imgtec.com

[dusan.simicic, 2017-05-23 12:17:39]

LGTM with nits

https://codereview.chromium.org/2892163002/diff/40001/src/mips64/macro-assemb...
File src/mips64/macro-assembler-mips64.cc (right):

https://codereview.chromium.org/2892163002/diff/40001/src/mips64/macro-assemb...
src/mips64/macro-assembler-mips64.cc:660: -rt.imm64_));  // No subiu instr, use
addiu(x, y, -imm).
Maybe this will be more readable with sequence:
int32_t imm = static_cast<int32_t>(-rt.imm64_);
addiu(rd, rs, imm);

https://codereview.chromium.org/2892163002/diff/40001/src/mips64/macro-assemb...
src/mips64/macro-assembler-mips64.cc:683: -rt.imm64_));  // No dsubiu instr, use
daddiu(x, y, -imm).
Same as above.

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

https://codereview.chromium.org/2892163002/diff/40001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:5065:
CHECK_EQ(assm.InstructionsGeneratedSince(&code_start), num_instr);
Add {} around CHECK_EQ().

https://codereview.chromium.org/2892163002/diff/40001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:5098: // This is int16 value and we can
load it using just daddiu.
Comments are usually placed before code sequence, this is a little confusing.

[ivica.bogosavljevic, 2017-05-23 12:32:05]

lgtm with nits

https://codereview.chromium.org/2892163002/diff/40001/src/mips64/macro-assemb...
File src/mips64/macro-assembler-mips64.cc (right):

https://codereview.chromium.org/2892163002/diff/40001/src/mips64/macro-assemb...
src/mips64/macro-assembler-mips64.cc:1747: if ((j.imm64_ & kUpper16MaskOf64) ==
0 && is_int16(j.imm64_ >> 32) &&
Is it possible to sorround the code with if(kArchVairant == kMips32R6) instead
of checking in every if?

[miran.karic, 2017-05-23 14:19:25]

https://codereview.chromium.org/2892163002/diff/40001/src/mips64/macro-assemb...
File src/mips64/macro-assembler-mips64.cc (right):

https://codereview.chromium.org/2892163002/diff/40001/src/mips64/macro-assemb...
src/mips64/macro-assembler-mips64.cc:660: -rt.imm64_));  // No subiu instr, use
addiu(x, y, -imm).
On 2017/05/23 12:17:38, dusan.simicic wrote:
> Maybe this will be more readable with sequence:
> int32_t imm = static_cast<int32_t>(-rt.imm64_);
> addiu(rd, rs, imm);

Acknowledged. I think not a big difference, no need to introduce another
variable.

https://codereview.chromium.org/2892163002/diff/40001/src/mips64/macro-assemb...
src/mips64/macro-assembler-mips64.cc:683: -rt.imm64_));  // No dsubiu instr, use
daddiu(x, y, -imm).
On 2017/05/23 12:17:38, dusan.simicic wrote:
> Same as above.

Acknowledged.

https://codereview.chromium.org/2892163002/diff/40001/src/mips64/macro-assemb...
src/mips64/macro-assembler-mips64.cc:1747: if ((j.imm64_ & kUpper16MaskOf64) ==
0 && is_int16(j.imm64_ >> 32) &&
On 2017/05/23 12:32:05, ivica.bogosavljevic wrote:
> Is it possible to sorround the code with if(kArchVairant == kMips32R6) instead
> of checking in every if?

Acknowledged. This would require adding return line to all if blocks and
adjusting the logic, I think it is better to leave as is.

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

https://codereview.chromium.org/2892163002/diff/40001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:5065:
CHECK_EQ(assm.InstructionsGeneratedSince(&code_start), num_instr);
On 2017/05/23 12:17:38, dusan.simicic wrote:
> Add {} around CHECK_EQ().

Done.

https://codereview.chromium.org/2892163002/diff/40001/test/cctest/test-assemb...
test/cctest/test-assembler-mips64.cc:5098: // This is int16 value and we can
load it using just daddiu.
On 2017/05/23 12:17:38, dusan.simicic wrote:
> Comments are usually placed before code sequence, this is a little confusing.

Acknowledged. I think that would be more confusing in this case as I want to
explain a test case. I think this is different than when we want to explain what
will a code sequence that follows do. Rather I adjusted the comments to make it
more clear they refer to the test case above.

[miran.karic, 2017-05-23 14:48:10]

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

[miran.karic, 2017-05-23 14:48:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
Ilija.Pavlovic@imgtec.com, ivica.bogosavljevic@imgtec.com,
dusan.simicic@imgtec.com
Link to the patchset: https://codereview.chromium.org/2892163002/#ps60001
(title: "Address comments")

[commit-bot: I haz the power, 2017-05-23 14:48:17]

CQ is trying da patch.

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

[miran.karic, 2017-05-23 14:48:26]

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

[miran.karic, 2017-05-23 14:50:19]

Description was changed from

==========
MIPS64: Add optimizations to li macro.

A number of improvements in mips64 load immediate macro is added per
suggestions from MIPS ART team. Also fix Subu macro, add a test for Subu
and make minor code adjustments.

BUG=
TEST=cctest/test-assembler-mips/li
==========

to

==========
MIPS64: Add optimizations to li macro.

A number of improvements in mips64 load immediate macro is added per
suggestions from MIPS ART team. Also fix Subu and Dsubu macro, add a
test for Subu and Dsubu and make minor code adjustments.

BUG=
TEST=cctest/test-assembler-mips/li_macro
     cctest/test-assembler-mips/Subu
     cctest/test-assembler-mips/Dsubu
==========

[miran.karic, 2017-05-23 14:50:19]

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

[miran.karic, 2017-05-23 14:51:05]

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

[commit-bot: I haz the power, 2017-05-23 14:51:17]

CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-05-23 16:01:43]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495551065215750,
"parent_rev": "8655861e0ab754fdf60ce6dd099905dd283de7b3", "commit_rev":
"cc1aae281253e8749d345064554cbd13a744d389"}

[commit-bot: I haz the power, 2017-05-23 16:01:51]

Description was changed from

==========
MIPS64: Add optimizations to li macro.

A number of improvements in mips64 load immediate macro is added per
suggestions from MIPS ART team. Also fix Subu and Dsubu macro, add a
test for Subu and Dsubu and make minor code adjustments.

BUG=
TEST=cctest/test-assembler-mips/li_macro
     cctest/test-assembler-mips/Subu
     cctest/test-assembler-mips/Dsubu
==========

to

==========
MIPS64: Add optimizations to li macro.

A number of improvements in mips64 load immediate macro is added per
suggestions from MIPS ART team. Also fix Subu and Dsubu macro, add a
test for Subu and Dsubu and make minor code adjustments.

BUG=
TEST=cctest/test-assembler-mips/li_macro
     cctest/test-assembler-mips/Subu
     cctest/test-assembler-mips/Dsubu

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

[commit-bot: I haz the power, 2017-05-23 16:01:52]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/v8/v8/+/cc1aae281253e8749d345064554cbd13a74...