MIPS: Fixing illegal use of at register

MIPS: Fix illegal use of at register

Fix illegal use of at register when ldc1 and sdc1 are called. Added dchecks to prevent such a usage.

TEST=mjsunit/asm/float64array-negative-offset(r6), mjsunit/asm/float64array-outofbounds(r6)

Committed: https://crrev.com/9e05ee74467e9ee14e53452f719782b5fe73d92d
Cr-Commit-Position: refs/heads/master@{#30670}

[Djordje.Pesic, 2015-08-31 11:06:31]

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

[paul.l..., 2015-08-31 22:56:34]

nit: please edit commit message to say "Fix illegal use of ....", rather than
Fixing (2 places)

I'm assuming from your change to lithium-codegen-mips that you ran tests, to
look for additional bad uses of 'at' in ldc1/sdc1. Please confirm this.

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

https://codereview.chromium.org/1323763002/diff/1/src/compiler/mips/code-gene...
src/compiler/mips/code-generator-mips.cc:308: __ asm_instr(result,
MemOperand(t8, 0));                                \
I think these temp regs should be kScratchReg or possibly kScratchReg2. t8 is
reserved for macro-assembler, and we should probably preserve that layering.

Also note that Assembler::sdc1() now uses t8 as a 2nd temporary. I think that
usage would still work, since we would only overwrite t8 after we have the
address, but it seems a bit fragile.

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

https://codereview.chromium.org/1323763002/diff/1/src/compiler/mips64/code-ge...
src/compiler/mips64/code-generator-mips64.cc:309: __ asm_instr(result,
MemOperand(t8, 0));                                \
I think these temp regs should be kScratchReg or possibly kScratchReg2. t8 is
reserved for macro-assembler, and we should probably preserve that layering.

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

https://codereview.chromium.org/1323763002/diff/1/src/mips/assembler-mips.cc#...
src/mips/assembler-mips.cc:2027: CHECK(!src.rm().is(at));
I think these should be DCHECK (here and below).

https://codereview.chromium.org/1323763002/diff/1/src/mips/assembler-mips.cc#...
src/mips/assembler-mips.cc:2084: mfhc1(t8, fd);
Note the use of t8 here. (t8 is reserved for macro-assembler use, but this is
probably ok, as ldc1 and sdc1 _should_ be moved to macro-assembler anyway (as
Ldc1() and Sdc1(), I will create an issue for that.)

It might make sense to add another DCHECK within this block:
DCHECK(!src.rm().is(t8));

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

https://codereview.chromium.org/1323763002/diff/1/src/mips64/assembler-mips64...
src/mips64/assembler-mips64.cc:2379: CHECK(!src.rm().is(at));
DCHECK() here and below.

[paul.l..., 2015-08-31 23:04:16]

Also, you should add a 'TEST=' line, to show at least one of the tests that fail
due to this error.

Also, there is no pre-existing convention for this, but since this test fails
for r6 only, I suggest putting that in () after the test. For example:

TEST=mjsunit/asm/float64array-negative-offset (r6)

[Djordje.Pesic, 2015-09-07 11:34:17]

I had one more issue in macroassembler-mips.cc. Switched t8 usage to t9

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

https://codereview.chromium.org/1323763002/diff/1/src/compiler/mips/code-gene...
src/compiler/mips/code-generator-mips.cc:308: __ asm_instr(result,
MemOperand(t8, 0));                                \
On 2015/08/31 22:56:34, paul.l... wrote:
> I think these temp regs should be kScratchReg or possibly kScratchReg2. t8 is
> reserved for macro-assembler, and we should probably preserve that layering.
> 
> Also note that Assembler::sdc1() now uses t8 as a 2nd temporary. I think that
> usage would still work, since we would only overwrite t8 after we have the
> address, but it seems a bit fragile.

Done.

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

https://codereview.chromium.org/1323763002/diff/1/src/compiler/mips64/code-ge...
src/compiler/mips64/code-generator-mips64.cc:309: __ asm_instr(result,
MemOperand(t8, 0));                                \
On 2015/08/31 22:56:34, paul.l... wrote:
> I think these temp regs should be kScratchReg or possibly kScratchReg2. t8 is
> reserved for macro-assembler, and we should probably preserve that layering.

Done.

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

https://codereview.chromium.org/1323763002/diff/1/src/mips/assembler-mips.cc#...
src/mips/assembler-mips.cc:2027: CHECK(!src.rm().is(at));
On 2015/08/31 22:56:34, paul.l... wrote:
> I think these should be DCHECK (here and below).

Done.

https://codereview.chromium.org/1323763002/diff/1/src/mips/assembler-mips.cc#...
src/mips/assembler-mips.cc:2084: mfhc1(t8, fd);
On 2015/08/31 22:56:34, paul.l... wrote:
> Note the use of t8 here. (t8 is reserved for macro-assembler use, but this is
> probably ok, as ldc1 and sdc1 _should_ be moved to macro-assembler anyway (as
> Ldc1() and Sdc1(), I will create an issue for that.)
> 
> It might make sense to add another DCHECK within this block:
> DCHECK(!src.rm().is(t8));

Done.

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

https://codereview.chromium.org/1323763002/diff/1/src/mips64/assembler-mips64...
src/mips64/assembler-mips64.cc:2379: CHECK(!src.rm().is(at));
On 2015/08/31 22:56:34, paul.l... wrote:
> DCHECK() here and below.

Done.

[paul.l..., 2015-09-08 15:48:41]

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

https://codereview.chromium.org/1323763002/diff/20001/src/mips/assembler-mips...
src/mips/assembler-mips.cc:2030: DCHECK(!src.rm().is(t8));
Why check for t8 here? You do not use it in this function.

This then requires the change to macro-asm LeaveExitFrame which is not really
needed.

[Djordje.Pesic, 2015-09-09 06:41:54]

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

https://codereview.chromium.org/1323763002/diff/20001/src/mips/assembler-mips...
src/mips/assembler-mips.cc:2030: DCHECK(!src.rm().is(t8));
On 2015/09/08 15:48:41, paul.l... wrote:
> Why check for t8 here? You do not use it in this function.
> 
> This then requires the change to macro-asm LeaveExitFrame which is not really
> needed.
DCHECK(!src.rm().is(t8)) removed. Macro-asm LeeaveExitFrame also changed to old
version with t8.

[paul.l..., 2015-09-10 04:09:43]

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

[paul.l..., 2015-09-10 04:09:43]

LGTM. Thanks!

[Djordje.Pesic, 2015-09-10 05:39:28]

The CQ bit was checked by djordje.pesic@imgtec.com

[commit-bot: I haz the power, 2015-09-10 05:39:35]

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

[commit-bot: I haz the power, 2015-09-10 06:02:31]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-09-10 06:02:45]

Patchset 3 (id:??) landed as
https://crrev.com/9e05ee74467e9ee14e53452f719782b5fe73d92d
Cr-Commit-Position: refs/heads/master@{#30670}