[turbofan] Add Float32(Max|Min) machine operators.

[turbofan] Add Float32(Max|Min) machine operators.

The new operators are implemented similar to the Float64(Max|Min) which
already exist. The purpose of the new operators is the implementation
of the F32Max and F32Min instructions in WebAssembly.

R=titzer@chromium.org, v8-arm-ports@googlegroups.com, v8-mips-ports@googlegroups.com

Committed: https://crrev.com/2027b0bed15e89583b31a8ea1a53cb85cccffda2
Cr-Commit-Position: refs/heads/master@{#38784}

[ahaas, 2016-08-17 11:02:31]

The CQ bit was checked by ahaas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-17 11:02:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[titzer, 2016-08-17 11:26:09]

https://codereview.chromium.org/2252863003/diff/1/src/compiler/machine-operat...
File src/compiler/machine-operator.cc (right):

https://codereview.chromium.org/2252863003/diff/1/src/compiler/machine-operat...
src/compiler/machine-operator.cc:167: V(Float32Max, Operator::kAssociative |
Operator::kCommutative, 2, 0, 1)    \
Are we sure these are associative?

https://codereview.chromium.org/2252863003/diff/1/test/cctest/compiler/test-r...
File test/cctest/compiler/test-run-machops.cc (right):

https://codereview.chromium.org/2252863003/diff/1/test/cctest/compiler/test-r...
test/cctest/compiler/test-run-machops.cc:3795: float fmax(float x, float y) {
Can you move this to a common place so that it can be shared? Also the double
version.

[commit-bot: I haz the power, 2016-08-17 11:36:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-17 11:36:32]

Dry run: This issue passed the CQ dry run.

[jbramley, 2016-08-17 12:02:57]

jacob.bramley@arm.com changed reviewers:
+ jacob.bramley@arm.com

[jbramley, 2016-08-17 12:02:57]

ARM and ARM64 look good, assuming that the semantics should match the Float64
variants. I think for ARMv8 (32-bit) we could improve things but that applies to
Float64 variants too and shouldn't hold up this patch.

https://codereview.chromium.org/2252863003/diff/1/src/compiler/machine-operat...
File src/compiler/machine-operator.cc (right):

https://codereview.chromium.org/2252863003/diff/1/src/compiler/machine-operat...
src/compiler/machine-operator.cc:167: V(Float32Max, Operator::kAssociative |
Operator::kCommutative, 2, 0, 1)    \
On 2016/08/17 11:26:09, titzer wrote:
> Are we sure these are associative?

They are associative, but they are only commutative if we don't care about which
NaN is propagated in the case where there are two NaNs.

Also note that since #37971, Float64(Min|Max) should share these properties too
(though that isn't related to this patch).

[ahaas, 2016-08-18 07:12:05]

The CQ bit was checked by ahaas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 07:12:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ahaas, 2016-08-18 07:13:20]

https://codereview.chromium.org/2252863003/diff/1/test/cctest/compiler/test-r...
File test/cctest/compiler/test-run-machops.cc (right):

https://codereview.chromium.org/2252863003/diff/1/test/cctest/compiler/test-r...
test/cctest/compiler/test-run-machops.cc:3795: float fmax(float x, float y) {
On 2016/08/17 at 11:26:09, titzer wrote:
> Can you move this to a common place so that it can be shared? Also the double
version.

Done.

[ahaas, 2016-08-18 07:18:17]

On 2016/08/17 at 12:02:57, jacob.bramley wrote:
> ARM and ARM64 look good, assuming that the semantics should match the Float64
variants. I think for ARMv8 (32-bit) we could improve things but that applies to
Float64 variants too and shouldn't hold up this patch.

Do you mean that on ARMv8 (32-bit) we could use the same instruction as on
ARM64? I agree that we should do that in a separate CL.

>
https://codereview.chromium.org/2252863003/diff/1/src/compiler/machine-operat...
> File src/compiler/machine-operator.cc (right):
> 
>
https://codereview.chromium.org/2252863003/diff/1/src/compiler/machine-operat...
> src/compiler/machine-operator.cc:167: V(Float32Max, Operator::kAssociative |
Operator::kCommutative, 2, 0, 1)    \
> On 2016/08/17 11:26:09, titzer wrote:
> > Are we sure these are associative?
> 
> They are associative, but they are only commutative if we don't care about
which NaN is propagated in the case where there are two NaNs.

Thanks for this observation, I missed the NaN case. NaN propagation was removed
from the WebAssembly spec recently, and it has never been part of the JavaScript
spec, so I think we can keep the commutativity. 

> Also note that since #37971, Float64(Min|Max) should share these properties
too (though that isn't related to this patch).

I agree that this should be done, and that it should be done in a separate CL.

[Jarin, 2016-08-18 07:42:41]

jarin@chromium.org changed reviewers:
+ jarin@chromium.org

[Jarin, 2016-08-18 07:42:42]

https://codereview.chromium.org/2252863003/diff/20001/src/compiler/typer.cc
File src/compiler/typer.cc (right):

https://codereview.chromium.org/2252863003/diff/20001/src/compiler/typer.cc#n...
src/compiler/typer.cc:2087: Type* Typer::Visitor::TypeFloat32Max(Node* node) {
return Type::Number(); }
Is this even reachable? If not, could you just say UNREACHABLE(); return
Type::Any();? (Same below.)

[commit-bot: I haz the power, 2016-08-18 08:02:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-18 08:02:30]

Dry run: This issue passed the CQ dry run.

[titzer, 2016-08-18 08:23:01]

https://codereview.chromium.org/2252863003/diff/20001/src/compiler/typer.cc
File src/compiler/typer.cc (right):

https://codereview.chromium.org/2252863003/diff/20001/src/compiler/typer.cc#n...
src/compiler/typer.cc:2087: Type* Typer::Visitor::TypeFloat32Max(Node* node) {
return Type::Number(); }
On 2016/08/18 07:42:42, Jarin wrote:
> Is this even reachable? If not, could you just say UNREACHABLE(); return
> Type::Any();? (Same below.)

If we leave it Number, it should be correct, and we won't have to touch it
if/when it does become reachable.

[jbramley, 2016-08-18 08:37:55]

On 2016/08/18 07:18:17, ahaas wrote:
> On 2016/08/17 at 12:02:57, jacob.bramley wrote:
> > ARM and ARM64 look good, assuming that the semantics should match the
Float64
> variants. I think for ARMv8 (32-bit) we could improve things but that applies
to
> Float64 variants too and shouldn't hold up this patch.
> 
> Do you mean that on ARMv8 (32-bit) we could use the same instruction as on
> ARM64? I agree that we should do that in a separate CL.

Not quite (since the ideal instruction isn't available) but we can get it down
to about five instructions.

[ahaas, 2016-08-18 12:54:02]

The CQ bit was checked by ahaas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 12:54:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ahaas, 2016-08-18 12:54:40]

https://codereview.chromium.org/2252863003/diff/20001/src/compiler/typer.cc
File src/compiler/typer.cc (right):

https://codereview.chromium.org/2252863003/diff/20001/src/compiler/typer.cc#n...
src/compiler/typer.cc:2087: Type* Typer::Visitor::TypeFloat32Max(Node* node) {
return Type::Number(); }
On 2016/08/18 at 07:42:42, Jarin wrote:
> Is this even reachable? If not, could you just say UNREACHABLE(); return
Type::Any();? (Same below.)

As discussed with jarin@ offline, I put UNREACHABLE() here because at the moment
this type information is not used at the moment, and it will be deleted soon
anyways.

[titzer, 2016-08-18 13:02:37]

On 2016/08/18 12:54:40, ahaas wrote:
> https://codereview.chromium.org/2252863003/diff/20001/src/compiler/typer.cc
> File src/compiler/typer.cc (right):
> 
>
https://codereview.chromium.org/2252863003/diff/20001/src/compiler/typer.cc#n...
> src/compiler/typer.cc:2087: Type* Typer::Visitor::TypeFloat32Max(Node* node) {
> return Type::Number(); }
> On 2016/08/18 at 07:42:42, Jarin wrote:
> > Is this even reachable? If not, could you just say UNREACHABLE(); return
> Type::Any();? (Same below.)
> 
> As discussed with jarin@ offline, I put UNREACHABLE() here because at the
moment
> this type information is not used at the moment, and it will be deleted soon
> anyways.

Seems like a timebomb for no good reason.

[titzer, 2016-08-18 13:02:39]

On 2016/08/18 12:54:40, ahaas wrote:
> https://codereview.chromium.org/2252863003/diff/20001/src/compiler/typer.cc
> File src/compiler/typer.cc (right):
> 
>
https://codereview.chromium.org/2252863003/diff/20001/src/compiler/typer.cc#n...
> src/compiler/typer.cc:2087: Type* Typer::Visitor::TypeFloat32Max(Node* node) {
> return Type::Number(); }
> On 2016/08/18 at 07:42:42, Jarin wrote:
> > Is this even reachable? If not, could you just say UNREACHABLE(); return
> Type::Any();? (Same below.)
> 
> As discussed with jarin@ offline, I put UNREACHABLE() here because at the
moment
> this type information is not used at the moment, and it will be deleted soon
> anyways.

Seems like a timebomb for no good reason.

[ahaas, 2016-08-18 13:06:41]

The CQ bit was checked by ahaas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 13:06:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ahaas, 2016-08-18 13:08:18]

On 2016/08/18 at 13:02:39, titzer wrote:
> On 2016/08/18 12:54:40, ahaas wrote:
> > https://codereview.chromium.org/2252863003/diff/20001/src/compiler/typer.cc
> > File src/compiler/typer.cc (right):
> > 
> >
https://codereview.chromium.org/2252863003/diff/20001/src/compiler/typer.cc#n...
> > src/compiler/typer.cc:2087: Type* Typer::Visitor::TypeFloat32Max(Node* node)
{
> > return Type::Number(); }
> > On 2016/08/18 at 07:42:42, Jarin wrote:
> > > Is this even reachable? If not, could you just say UNREACHABLE(); return
> > Type::Any();? (Same below.)
> > 
> > As discussed with jarin@ offline, I put UNREACHABLE() here because at the
moment
> > this type information is not used at the moment, and it will be deleted soon
> > anyways.
> 
> Seems like a timebomb for no good reason.

It does not matter too much I think because it is dead code at the moment, and
jarin@ said that it will be removed anyways soon. I added a type again now.

[titzer, 2016-08-18 13:10:57]

On 2016/08/18 13:08:18, ahaas wrote:
> On 2016/08/18 at 13:02:39, titzer wrote:
> > On 2016/08/18 12:54:40, ahaas wrote:
> > >
https://codereview.chromium.org/2252863003/diff/20001/src/compiler/typer.cc
> > > File src/compiler/typer.cc (right):
> > > 
> > >
>
https://codereview.chromium.org/2252863003/diff/20001/src/compiler/typer.cc#n...
> > > src/compiler/typer.cc:2087: Type* Typer::Visitor::TypeFloat32Max(Node*
node)
> {
> > > return Type::Number(); }
> > > On 2016/08/18 at 07:42:42, Jarin wrote:
> > > > Is this even reachable? If not, could you just say UNREACHABLE(); return
> > > Type::Any();? (Same below.)
> > > 
> > > As discussed with jarin@ offline, I put UNREACHABLE() here because at the
> moment
> > > this type information is not used at the moment, and it will be deleted
soon
> > > anyways.
> > 
> > Seems like a timebomb for no good reason.
> 
> It does not matter too much I think because it is dead code at the moment, and
> jarin@ said that it will be removed anyways soon. I added a type again now.

Either is fine. Sorry to bikeshed.

[commit-bot: I haz the power, 2016-08-18 13:27:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-18 13:27:21]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[ahaas, 2016-08-18 14:07:55]

The CQ bit was checked by ahaas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 14:08:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ahaas, 2016-08-18 14:08:45]

The CQ bit was checked by ahaas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 14:08:53]

Dry run: 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, 2016-08-18 14:53:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-18 14:53:04]

Dry run: This issue passed the CQ dry run.

[titzer, 2016-08-22 13:08:53]

lgtm

https://codereview.chromium.org/2252863003/diff/100001/src/compiler/typer.cc
File src/compiler/typer.cc (right):

https://codereview.chromium.org/2252863003/diff/100001/src/compiler/typer.cc#...
src/compiler/typer.cc:2087: Type* Typer::Visitor::TypeFloat32Max(Node* node) {
return Type::Any(); }
Should be the same as Float64Min/Max

[ahaas, 2016-08-22 13:14:53]

On 2016/08/22 at 13:08:53, titzer wrote:
> lgtm
> 
> https://codereview.chromium.org/2252863003/diff/100001/src/compiler/typer.cc
> File src/compiler/typer.cc (right):
> 
>
https://codereview.chromium.org/2252863003/diff/100001/src/compiler/typer.cc#...
> src/compiler/typer.cc:2087: Type* Typer::Visitor::TypeFloat32Max(Node* node) {
return Type::Any(); }
> Should be the same as Float64Min/Max

Done.

[ahaas, 2016-08-22 13:15:17]

The CQ bit was checked by ahaas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-22 13:15:25]

Dry run: 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, 2016-08-22 13:42:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-22 13:42:24]

Dry run: This issue passed the CQ dry run.

[ahaas, 2016-08-22 13:48:12]

The CQ bit was checked by ahaas@chromium.org

[ahaas, 2016-08-22 13:48:13]

The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org
Link to the patchset: https://codereview.chromium.org/2252863003/#ps120001
(title: "Type is number now.")

[commit-bot: I haz the power, 2016-08-22 13:48:23]

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, 2016-08-22 13:50:32]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-08-22 13:50:59]

Description was changed from

==========
[turbofan] Add Float32(Max|Min) machine operators.

The new operators are implemented similar to the Float64(Max|Min) which
already exist. The purpose of the new operators is the implementation
of the F32Max and F32Min instructions in WebAssembly.

R=titzer@chromium.org, v8-arm-ports@googlegroups.com,
v8-mips-ports@googlegroups.com
==========

to

==========
[turbofan] Add Float32(Max|Min) machine operators.

The new operators are implemented similar to the Float64(Max|Min) which
already exist. The purpose of the new operators is the implementation
of the F32Max and F32Min instructions in WebAssembly.

R=titzer@chromium.org, v8-arm-ports@googlegroups.com,
v8-mips-ports@googlegroups.com

Committed: https://crrev.com/2027b0bed15e89583b31a8ea1a53cb85cccffda2
Cr-Commit-Position: refs/heads/master@{#38784}
==========

[commit-bot: I haz the power, 2016-08-22 13:51:01]

Patchset 7 (id:??) landed as
https://crrev.com/2027b0bed15e89583b31a8ea1a53cb85cccffda2
Cr-Commit-Position: refs/heads/master@{#38784}