X87: [wasm] Use the Float64Max/Min machine operators to implement F64Max/Min.

X87: [wasm] Use the Float64Max/Min machine operators to implement F64Max/Min.

  port 96c90f6c64d7a9e4a49491c4a80debd78b376bcf (r38410)

  original commit message:
  I had to adjust Float64Max/Min on x64 slightly to return the default
  wasm NaN (0x7FF0000000000000) instead of the all-ones NaN
  (0xFFFFFFFFFFFFFFFF).

BUG=

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

[zhengxing.li, 2016-08-11 03:24:49]

zhengxing.li@intel.com changed reviewers:
+ ahaas@chromium.org, chunyang.dai@intel.com, weiliang.lin@intel.com

[zhengxing.li, 2016-08-11 03:24:50]

PTAL, thanks!

[ahaas, 2016-08-11 06:39:01]

https://codereview.chromium.org/2233273002/diff/1/src/compiler/x87/code-gener...
File src/compiler/x87/code-generator-x87.cc (left):

https://codereview.chromium.org/2233273002/diff/1/src/compiler/x87/code-gener...
src/compiler/x87/code-generator-x87.cc:203: __ push(Immediate(0xffffffff));
I think you could also just push the default NaN instead of calculating it. The
default NaN for float64 is 0xfff8000000000000. For float32 it is 0xffc00000.

[zhengxing.li, 2016-08-11 07:44:26]

On 2016/08/11 06:39:01, ahaas wrote:
>
https://codereview.chromium.org/2233273002/diff/1/src/compiler/x87/code-gener...
> File src/compiler/x87/code-generator-x87.cc (left):
> 
>
https://codereview.chromium.org/2233273002/diff/1/src/compiler/x87/code-gener...
> src/compiler/x87/code-generator-x87.cc:203: __ push(Immediate(0xffffffff));
> I think you could also just push the default NaN instead of calculating it.
The
> default NaN for float64 is 0xfff8000000000000. For float32 it is 0xffc00000.

Hi, ahaas:

Thanks for your nice comment, I have updated the CL. please help to review it
again!

Thanks!

[ahaas, 2016-08-11 08:06:12]

I am confused by the order in which you push the words of the float64 NaN. Other
than that lgtm.

https://codereview.chromium.org/2233273002/diff/20001/src/crankshaft/x87/lith...
File src/crankshaft/x87/lithium-codegen-x87.cc (right):

https://codereview.chromium.org/2233273002/diff/20001/src/crankshaft/x87/lith...
src/crankshaft/x87/lithium-codegen-x87.cc:4738: __ push(Immediate(0xfff80000));
In the old code the low word "0xffffffff" was push first and the high word
"0x7fffffff" was pushed second. It seems to me that in the new code the low word
"0x00000000" is pushed second and the high word "0xfff80000" is pushed first.
Are you sure that you push the words in the right order here?

[zhengxing.li, 2016-08-11 08:14:51]

On 2016/08/11 08:06:12, ahaas wrote:
> I am confused by the order in which you push the words of the float64 NaN.
Other
> than that lgtm.
> 
>
https://codereview.chromium.org/2233273002/diff/20001/src/crankshaft/x87/lith...
> File src/crankshaft/x87/lithium-codegen-x87.cc (right):
> 
>
https://codereview.chromium.org/2233273002/diff/20001/src/crankshaft/x87/lith...
> src/crankshaft/x87/lithium-codegen-x87.cc:4738: __
push(Immediate(0xfff80000));
> In the old code the low word "0xffffffff" was push first and the high word
> "0x7fffffff" was pushed second. It seems to me that in the new code the low
word
> "0x00000000" is pushed second and the high word "0xfff80000" is pushed first.
> Are you sure that you push the words in the right order here?

The old order was wrong but it still produced a valid NaN.

The order in this CL should be right as Intel CPU is little endian.

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

On 2016/08/11 at 08:14:51, zhengxing.li wrote:
> On 2016/08/11 08:06:12, ahaas wrote:
> > I am confused by the order in which you push the words of the float64 NaN.
Other
> > than that lgtm.
> > 
> >
https://codereview.chromium.org/2233273002/diff/20001/src/crankshaft/x87/lith...
> > File src/crankshaft/x87/lithium-codegen-x87.cc (right):
> > 
> >
https://codereview.chromium.org/2233273002/diff/20001/src/crankshaft/x87/lith...
> > src/crankshaft/x87/lithium-codegen-x87.cc:4738: __
push(Immediate(0xfff80000));
> > In the old code the low word "0xffffffff" was push first and the high word
> > "0x7fffffff" was pushed second. It seems to me that in the new code the low
word
> > "0x00000000" is pushed second and the high word "0xfff80000" is pushed
first.
> > Are you sure that you push the words in the right order here?
> 
> The old order was wrong but it still produced a valid NaN.
> 
> The order in this CL should be right as Intel CPU is little endian.

Makes sense, thanks!

[zhengxing.li, 2016-08-11 08:20:41]

On 2016/08/11 08:18:18, ahaas wrote:
> On 2016/08/11 at 08:14:51, zhengxing.li wrote:
> > On 2016/08/11 08:06:12, ahaas wrote:
> > > I am confused by the order in which you push the words of the float64 NaN.
> Other
> > > than that lgtm.
> > > 
> > >
>
https://codereview.chromium.org/2233273002/diff/20001/src/crankshaft/x87/lith...
> > > File src/crankshaft/x87/lithium-codegen-x87.cc (right):
> > > 
> > >
>
https://codereview.chromium.org/2233273002/diff/20001/src/crankshaft/x87/lith...
> > > src/crankshaft/x87/lithium-codegen-x87.cc:4738: __
> push(Immediate(0xfff80000));
> > > In the old code the low word "0xffffffff" was push first and the high word
> > > "0x7fffffff" was pushed second. It seems to me that in the new code the
low
> word
> > > "0x00000000" is pushed second and the high word "0xfff80000" is pushed
> first.
> > > Are you sure that you push the words in the right order here?
> > 
> > The old order was wrong but it still produced a valid NaN.
> > 
> > The order in this CL should be right as Intel CPU is little endian.
> 
> Makes sense, thanks!

Thanks!

[Weiliang, 2016-08-11 08:23:34]

On 2016/08/11 08:20:41, zhengxing.li wrote:
> On 2016/08/11 08:18:18, ahaas wrote:
> > On 2016/08/11 at 08:14:51, zhengxing.li wrote:
> > > On 2016/08/11 08:06:12, ahaas wrote:
> > > > I am confused by the order in which you push the words of the float64
NaN.
> > Other
> > > > than that lgtm.
> > > > 
> > > >
> >
>
https://codereview.chromium.org/2233273002/diff/20001/src/crankshaft/x87/lith...
> > > > File src/crankshaft/x87/lithium-codegen-x87.cc (right):
> > > > 
> > > >
> >
>
https://codereview.chromium.org/2233273002/diff/20001/src/crankshaft/x87/lith...
> > > > src/crankshaft/x87/lithium-codegen-x87.cc:4738: __
> > push(Immediate(0xfff80000));
> > > > In the old code the low word "0xffffffff" was push first and the high
word
> > > > "0x7fffffff" was pushed second. It seems to me that in the new code the
> low
> > word
> > > > "0x00000000" is pushed second and the high word "0xfff80000" is pushed
> > first.
> > > > Are you sure that you push the words in the right order here?
> > > 
> > > The old order was wrong but it still produced a valid NaN.
> > > 
> > > The order in this CL should be right as Intel CPU is little endian.
> > 
> > Makes sense, thanks!
> 
> Thanks!

lgtm

[zhengxing.li, 2016-08-11 08:24:15]

The CQ bit was checked by zhengxing.li@intel.com

[commit-bot: I haz the power, 2016-08-11 08:24:27]

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-11 08:47:08]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-08-11 08:47:26]

Description was changed from

==========
X87: [wasm] Use the Float64Max/Min machine operators to implement F64Max/Min.

  port 96c90f6c64d7a9e4a49491c4a80debd78b376bcf (r38410)

  original commit message:
  I had to adjust Float64Max/Min on x64 slightly to return the default
  wasm NaN (0x7FF0000000000000) instead of the all-ones NaN
  (0xFFFFFFFFFFFFFFFF).

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

to

==========
X87: [wasm] Use the Float64Max/Min machine operators to implement F64Max/Min.

  port 96c90f6c64d7a9e4a49491c4a80debd78b376bcf (r38410)

  original commit message:
  I had to adjust Float64Max/Min on x64 slightly to return the default
  wasm NaN (0x7FF0000000000000) instead of the all-ones NaN
  (0xFFFFFFFFFFFFFFFF).

BUG=

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

[commit-bot: I haz the power, 2016-08-11 08:47:27]

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