MIPS: [turbofan] Properly implement Float64/32 Min/Max instructions.

MIPS: [turbofan] Properly implement Float64/32 Min/Max instructions.

TEST=cctest/test-run-machops/Float(64|32)MaxP, Float(64|32)MinP,
       unittests/InstructionSelectorTest.Float64Min|Max
BUG=v8:4206
LOG=N

Committed: https://crrev.com/8ae7c9abc350d2df579c412d27ed71abefdcca65
Cr-Commit-Position: refs/heads/master@{#31806}

[dusan.milosavljevic, 2015-11-02 20:54:34]

dusan.m.milosavljevic@gmail.com changed reviewers:
+ akos.palfi@imgtec.com, bmeurer@chromium.org, paul.lind@imgtec.com

[dusan.milosavljevic, 2015-11-02 20:54:35]

PTAL.

[paul.l..., 2015-11-02 22:35:12]

r2 fails regress-undefined-nan, regress-undefined-nan3 on both mips32 & 64. r6
passes all, but I am not seeing how.

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

https://codereview.chromium.org/1419753008/diff/1/src/compiler/mips/code-gene...
src/compiler/mips/code-generator-mips.cc:727: __ cmp(OLT, L, kScratchDoubleReg,
i.InputDoubleRegister(0),
You're putting the result of cmp in scratch, but due to semantics of sel_d() it
needs to be in output register.

https://codereview.chromium.org/1419753008/diff/1/src/compiler/mips/code-gene...
src/compiler/mips/code-generator-mips.cc:730: kScratchDoubleReg);
s/b: sel_d(i.OutputDoubleRegister(),
i.InputDoubleRegister(0),i.InputDoubleRegister(1))

from manual: FPR[fd] 􏰀= FPR[fd].bit0 ? FPR[ft] : FPR[fs]

I don't see how this is working, unless we have insufficient test coverage.

https://codereview.chromium.org/1419753008/diff/1/src/compiler/mips/code-gene...
src/compiler/mips/code-generator-mips.cc:734: __
movt_d(i.OutputSingleRegister(), i.InputDoubleRegister(1));
All the Float64 output regs are show as OutputSingleRegister(), but should be
OutputDoubleRegister().

I do realize they are aliases, but we should use them consistently.

https://codereview.chromium.org/1419753008/diff/1/src/compiler/mips/code-gene...
src/compiler/mips/code-generator-mips.cc:741: __ cmp(OLT, L, kScratchDoubleReg,
i.InputDoubleRegister(0),
nit: we have asm aliases for cmp_d() and cmp_s(), you should use those here, and
below. Using the raw cmp() with L & W types is correct per the encodings, but
its a cognitive dissonance, since every other usage of L & W are for ints, and D
& S are the common usage for floats. (above, here, and below)

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

https://codereview.chromium.org/1419753008/diff/1/src/compiler/mips64/code-ge...
src/compiler/mips64/code-generator-mips64.cc:791: if (kArchVariant == kMips64r6)
{
Please see comments in mips32 version, they all apply here.

[dusan.milosavljevic, 2015-11-03 01:51:05]

As we concluded in offline discussion, the regress-nan-undefined failures are
not related to this CL.

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

https://codereview.chromium.org/1419753008/diff/1/src/compiler/mips/code-gene...
src/compiler/mips/code-generator-mips.cc:727: __ cmp(OLT, L, kScratchDoubleReg,
i.InputDoubleRegister(0),
On 2015/11/02 22:35:11, paul.l... wrote:
> You're putting the result of cmp in scratch, but due to semantics of sel_d()
it
> needs to be in output register.

Done.

https://codereview.chromium.org/1419753008/diff/1/src/compiler/mips/code-gene...
src/compiler/mips/code-generator-mips.cc:730: kScratchDoubleReg);
On 2015/11/02 22:35:11, paul.l... wrote:
> s/b: sel_d(i.OutputDoubleRegister(),
> i.InputDoubleRegister(0),i.InputDoubleRegister(1))
> 
> from manual: FPR[fd] 􏰀= FPR[fd].bit0 ? FPR[ft] : FPR[fs]
> 
> I don't see how this is working, unless we have insufficient test coverage.

Done.

Different instruction from other sel. instruction, i assumed they are similar,
but this one is not.

https://codereview.chromium.org/1419753008/diff/1/src/compiler/mips/code-gene...
src/compiler/mips/code-generator-mips.cc:734: __
movt_d(i.OutputSingleRegister(), i.InputDoubleRegister(1));
On 2015/11/02 22:35:12, paul.l... wrote:
> All the Float64 output regs are show as OutputSingleRegister(), but should be
> OutputDoubleRegister().
> 
> I do realize they are aliases, but we should use them consistently.

Done.

https://codereview.chromium.org/1419753008/diff/1/src/compiler/mips/code-gene...
src/compiler/mips/code-generator-mips.cc:741: __ cmp(OLT, L, kScratchDoubleReg,
i.InputDoubleRegister(0),
On 2015/11/02 22:35:11, paul.l... wrote:
> nit: we have asm aliases for cmp_d() and cmp_s(), you should use those here,
and
> below. Using the raw cmp() with L & W types is correct per the encodings, but
> its a cognitive dissonance, since every other usage of L & W are for ints, and
D
> & S are the common usage for floats. (above, here, and below)

Done.

[dusan.milosavljevic, 2015-11-03 02:33:46]

Description was changed from

==========
MIPS: [turbofan] Properly implement Float64/32 Min/Max instructions.

TEST=unittests/InstructionSelectorTest.Float64Min/Max
BUG=
==========

to

==========
MIPS: [turbofan] Properly implement Float64/32 Min/Max instructions.

TEST=cctest/test-run-machops/Float64|32MaxP, Float64|32MinP
          unittests/InstructionSelectorTest.Float64Min/Max
BUG=v8:4206
LOG=N
==========

[dusan.milosavljevic, 2015-11-03 02:34:25]

Description was changed from

==========
MIPS: [turbofan] Properly implement Float64/32 Min/Max instructions.

TEST=cctest/test-run-machops/Float64|32MaxP, Float64|32MinP
          unittests/InstructionSelectorTest.Float64Min/Max
BUG=v8:4206
LOG=N
==========

to

==========
MIPS: [turbofan] Properly implement Float64/32 Min/Max instructions.

TEST=cctest/test-run-machops/Float64|32MaxP, Float64|32MinP,
unittests/InstructionSelectorTest.Float64Min/Max
BUG=v8:4206
LOG=N
==========

[dusan.milosavljevic, 2015-11-03 02:34:40]

Description was changed from

==========
MIPS: [turbofan] Properly implement Float64/32 Min/Max instructions.

TEST=cctest/test-run-machops/Float64|32MaxP, Float64|32MinP,
unittests/InstructionSelectorTest.Float64Min/Max
BUG=v8:4206
LOG=N
==========

to

==========
MIPS: [turbofan] Properly implement Float64/32 Min/Max instructions.

TEST=cctest/test-run-machops/Float64|32MaxP, Float64|32MinP,
          unittests/InstructionSelectorTest.Float64Min/Max
BUG=v8:4206
LOG=N
==========

[dusan.milosavljevic, 2015-11-03 02:37:39]

Description was changed from

==========
MIPS: [turbofan] Properly implement Float64/32 Min/Max instructions.

TEST=cctest/test-run-machops/Float64|32MaxP, Float64|32MinP,
          unittests/InstructionSelectorTest.Float64Min/Max
BUG=v8:4206
LOG=N
==========

to

==========
MIPS: [turbofan] Properly implement Float64/32 Min/Max instructions.

TEST=cctest/test-run-machops/Float(64|32)MaxP, Float(64|32)MinP,
       unittests/InstructionSelectorTest.Float64Min|Max
BUG=v8:4206
LOG=N
==========

[Benedikt Meurer, 2015-11-03 05:39:00]

https://codereview.chromium.org/1419753008/diff/40001/src/compiler/machine-op...
File src/compiler/machine-operator.h (right):

https://codereview.chromium.org/1419753008/diff/40001/src/compiler/machine-op...
src/compiler/machine-operator.h:239: bool HasFloat32Max() const { return flags_
& kFloat32Max; }
You don't need these functions. Just use Float32Max().IsSupported() instead.

https://codereview.chromium.org/1419753008/diff/40001/src/compiler/machine-op...
src/compiler/machine-operator.h:240: bool HasFloat64Max() const { return flags_
& kFloat64Max; }
Same as above.

https://codereview.chromium.org/1419753008/diff/40001/src/compiler/machine-op...
src/compiler/machine-operator.h:245: bool HasFloat32Min() const { return flags_
& kFloat32Min; }
Same as above.

[dusan.milosavljevic, 2015-11-03 13:48:35]

https://codereview.chromium.org/1419753008/diff/40001/src/compiler/machine-op...
File src/compiler/machine-operator.h (right):

https://codereview.chromium.org/1419753008/diff/40001/src/compiler/machine-op...
src/compiler/machine-operator.h:239: bool HasFloat32Max() const { return flags_
& kFloat32Max; }
On 2015/11/03 05:39:00, Benedikt Meurer wrote:
> You don't need these functions. Just use Float32Max().IsSupported() instead.

Right, exactly what I need. Thank you.

https://codereview.chromium.org/1419753008/diff/40001/src/compiler/machine-op...
src/compiler/machine-operator.h:239: bool HasFloat32Max() const { return flags_
& kFloat32Max; }
On 2015/11/03 05:39:00, Benedikt Meurer wrote:
> You don't need these functions. Just use Float32Max().IsSupported() instead.

Done.

https://codereview.chromium.org/1419753008/diff/40001/src/compiler/machine-op...
src/compiler/machine-operator.h:245: bool HasFloat32Min() const { return flags_
& kFloat32Min; }
On 2015/11/03 05:39:00, Benedikt Meurer wrote:
> Same as above.

Done.

[paul.l..., 2015-11-03 20:32:28]

LGTM. Benedikt, could you please take another look? We need your OK for change
to test-run-machops.cc

[Benedikt Meurer, 2015-11-04 02:49:08]

Thanks. LGTM

[dusan.milosavljevic, 2015-11-04 20:21:25]

The CQ bit was checked by dusan.m.milosavljevic@gmail.com

[commit-bot: I haz the power, 2015-11-04 20:21:39]

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

[commit-bot: I haz the power, 2015-11-04 21:02:54]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-04 21:03:34]

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