MIPS: Use PC realitive instructions on r6.

MIPS: Use PC relative instructions on r6.

BUG=

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

[ivica.bogosavljevic, 2016-01-25 11:40:03]

ivica.bogosavljevic@imgtec.com changed reviewers:
+ ivica.bogosavljevic@imgtec.com

[ivica.bogosavljevic, 2016-01-25 11:40:03]

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

https://codereview.chromium.org/1628453002/diff/1/src/compiler/mips64/code-ge...
src/compiler/mips64/code-generator-mips64.cc:1660: if (kArchVariant >=
kMips64r6) {
Also,  I am not sure if this is a best way to do this. What do you think,
instead of this, we introduce a macro
#define MipsArchVairantGreaterEqual(X) or something similar and use it?

https://codereview.chromium.org/1628453002/diff/1/src/compiler/mips64/code-ge...
src/compiler/mips64/code-generator-mips64.cc:1663: if ((masm()->pc_offset() & 7)
== 0) {
I think you should call Align(8) + additional nop() after this instead of this,
because this code wouldn't work correctly if previous instruction were compact
branch. In that case calling Aling would emit the instruction pending for
forbidden slot.

https://codereview.chromium.org/1628453002/diff/1/test/cctest/test-macro-asse...
File test/cctest/test-macro-assembler-mips64.cc (right):

https://codereview.chromium.org/1628453002/diff/1/test/cctest/test-macro-asse...
test/cctest/test-macro-assembler-mips64.cc:329: // following block before the
first dd().
Already made a remark

[balazs.kilvady, 2016-01-25 12:46:17]

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

https://codereview.chromium.org/1628453002/diff/1/src/compiler/mips64/code-ge...
src/compiler/mips64/code-generator-mips64.cc:1660: if (kArchVariant >=
kMips64r6) {
On 2016/01/25 11:40:03, ivica.bogosavljevic wrote:
> Also,  I am not sure if this is a best way to do this. What do you think,
> instead of this, we introduce a macro
> #define MipsArchVairantGreaterEqual(X) or something similar and use it?

 if (VERSION >= 7) {}
code is quite common in several projects so I am OK with any kind of similar
solution just we have to agree and follow the agrement :)

https://codereview.chromium.org/1628453002/diff/1/src/compiler/mips64/code-ge...
src/compiler/mips64/code-generator-mips64.cc:1663: if ((masm()->pc_offset() & 7)
== 0) {
On 2016/01/25 11:40:03, ivica.bogosavljevic wrote:
> I think you should call Align(8) + additional nop() after this instead of
this,
> because this code wouldn't work correctly if previous instruction were compact
> branch. In that case calling Aling would emit the instruction pending for
> forbidden slot.

I don't think so. We don't need 1 or 2 nops as addiupc is not a CTI. In the case
of a previous compact branch it does not matter if there is a nop before
addiupc.

https://codereview.chromium.org/1628453002/diff/1/test/cctest/test-macro-asse...
File test/cctest/test-macro-assembler-mips64.cc (right):

https://codereview.chromium.org/1628453002/diff/1/test/cctest/test-macro-asse...
test/cctest/test-macro-assembler-mips64.cc:329: // following block before the
first dd().
On 2016/01/25 11:40:03, ivica.bogosavljevic wrote:
> Already made a remark

So do I :)

[ivica.bogosavljevic, 2016-01-26 08:32:29]

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

https://codereview.chromium.org/1628453002/diff/1/src/compiler/mips64/code-ge...
src/compiler/mips64/code-generator-mips64.cc:1663: if ((masm()->pc_offset() & 7)
== 0) {
I am not sure what to think about this. On one hand, you save one instruction if
you do it this way. On the other hand, if somebody new would use this code as a
template to implement unalign, he would run into problems if instruction
following this unalign were a branch-compact.

Personally, I would prefer to have some Unalign macro or something similar.
Unless the code is used really often, saving one instruction doesn't bring a lot
and has a potential to create problems in the future.

On 2016/01/25 12:46:17, balazs.kilvady wrote:
> On 2016/01/25 11:40:03, ivica.bogosavljevic wrote:
> > I think you should call Align(8) + additional nop() after this instead of
> this,
> > because this code wouldn't work correctly if previous instruction were
compact
> > branch. In that case calling Aling would emit the instruction pending for
> > forbidden slot.
> 
> I don't think so. We don't need 1 or 2 nops as addiupc is not a CTI. In the
case
> of a previous compact branch it does not matter if there is a nop before
> addiupc.

[balazs.kilvady, 2016-01-26 18:40:05]

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

https://codereview.chromium.org/1628453002/diff/1/src/compiler/mips64/code-ge...
src/compiler/mips64/code-generator-mips64.cc:1663: if ((masm()->pc_offset() & 7)
== 0) {
On 2016/01/26 08:32:29, ivica.bogosavljevic wrote:
> I am not sure what to think about this. On one hand, you save one instruction
if
> you do it this way. On the other hand, if somebody new would use this code as
a
> template to implement unalign, he would run into problems if instruction
> following this unalign were a branch-compact.
I see.

> Personally, I would prefer to have some Unalign macro or something similar.
> Unless the code is used really often, saving one instruction doesn't bring a
lot
> and has a potential to create problems in the future.
I created a macro to hide (behind MacroAssembler) the dangerous align code. I
don't think we win too much with this solution but fun to implement and I learn
how to use PC relative instruction(s) :)

[ivica.bogosavljevic, 2016-01-27 10:30:00]

lgtm

[balazs.kilvady, 2016-01-27 15:46:19]

not lgtm

[balazs.kilvady, 2016-01-27 18:03:13]

balazs.kilvady@imgtec.com changed reviewers:
+ akos.palfi@imgtec.com, gergely.kis@imgtec.com, marija.antic@imgtec.com

[balazs.kilvady, 2016-01-29 10:25:35]

NOT LGTM

[balazs.kilvady, 2016-01-29 12:56:03]

Rebased and macro fixed. PTAL.

[balazs.kilvady, 2016-02-01 16:56:52]

Description was changed from

==========
MIPS: Use PC realitive instructions on r6.

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

to

==========
MIPS: Use PC relative instructions on r6.

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

[akos.palfi.imgtec, 2016-02-03 13:01:36]

Nice, LGTM!

[balazs.kilvady, 2016-02-03 13:12:36]

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

[balazs.kilvady, 2016-02-03 13:12:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
ivica.bogosavljevic@imgtec.com
Link to the patchset: https://codereview.chromium.org/1628453002/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2016-02-03 13:12:43]

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

[commit-bot: I haz the power, 2016-02-03 13:31:04]

Description was changed from

==========
MIPS: Use PC relative instructions on r6.

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

to

==========
MIPS: Use PC relative instructions on r6.

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

[commit-bot: I haz the power, 2016-02-03 13:31:05]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-02-03 13:31:36]

Description was changed from

==========
MIPS: Use PC relative instructions on r6.

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

to

==========
MIPS: Use PC relative instructions on r6.

BUG=

Committed: https://crrev.com/ebac85c0a4ada986dad82c1b6ed3aa8ed5dfe309
Cr-Commit-Position: refs/heads/master@{#33703}
==========

[commit-bot: I haz the power, 2016-02-03 13:31:36]

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