[X87] [crankshaft] Fix Math.max(-0, 0) bug.

[X87] [crankshaft] Fix Math.max(-0, 0) bug.

  This CL fixed one bug in crankshaft compiler for Math.max(-0, 0).

BUG=

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

[zhengxing.li, 2016-07-25 09:00:23]

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

[zhengxing.li, 2016-07-25 09:00:23]

PTAL, thanks!

[Jakob Kummerow, 2016-07-25 11:31:55]

Drive-by comment: This CL's description doesn't provide much information beyond
the "x87" and "crankshaft" keywords. How about something like this:

[x87][crankshaft] Fix Math.max() for -0 inputs

Then someone comparing a given bug with recent CLs (that may have either
introduced or fixed it) can more easily tell whether this CL is potentially
related.

Also, while I sympathize with the pain of dealing with hard-to-reproduce bugs,
that tends to make it all the more important to add a regression test (which
would also have the side effect of documenting very clearly what situation was
broken before this fix).

[zhengxing.li, 2016-07-26 01:42:49]

On 2016/07/25 11:31:55, Jakob wrote:
> Drive-by comment: This CL's description doesn't provide much information
beyond
> the "x87" and "crankshaft" keywords. How about something like this:
> 
> [x87][crankshaft] Fix Math.max() for -0 inputs
> 
> Then someone comparing a given bug with recent CLs (that may have either
> introduced or fixed it) can more easily tell whether this CL is potentially
> related.
> 
> Also, while I sympathize with the pain of dealing with hard-to-reproduce bugs,
> that tends to make it all the more important to add a regression test (which
> would also have the side effect of documenting very clearly what situation was
> broken before this fix).

Thanks Jakob!

I will update this CL according to your comment.

For regression test, the Math.max(-0, 0) test is in result-table-max.js already.
What's make the bug hard-to-reproduce is the Math.max() works in FC mode most of
time.
So, It's need to add another Math.max(-0, 0) test?

Thanks!

[zhengxing.li, 2016-07-26 02:14:04]

Description was changed from

==========
[X87] [crankshaft] Fix one V8 x87 code generation bug in crankshaft compiler.

  This CL fixed one bug in crankshaft compiler which was hard to be reproduced.

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

to

==========
[X87] [crankshaft] Fix Math.max(-0, 0) bug.

  This CL fixed one bug in crankshaft compiler for Math.max(-0, 0).

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

[zhengxing.li, 2016-07-26 08:57:42]

zhengxing.li@intel.com changed reviewers:
+ jkummerow@chromium.org

[Jakob Kummerow, 2016-07-26 09:51:27]

On 2016/07/26 01:42:49, zhengxing.li wrote:
> For regression test, the Math.max(-0, 0) test is in result-table-max.js
already.
> What's make the bug hard-to-reproduce is the Math.max() works in FC mode most
of
> time.
> So, It's need to add another Math.max(-0, 0) test?

In the x87 port you can do whatever you want. If you don't think you need a
test, go ahead and land without one.

The general rule of thumb is: if a bug is discovered that the existing test
suite did not expose, then a test should be added that makes sure that the
affected code path works correctly. A minimum requirement for such a test is
that it would have failed before the fix (otherwise, how can you be sure that
the fix actually fixes the problem, and that the problem won't come back next
time someone else touches the code?).

[zhengxing.li, 2016-07-26 10:06:44]

On 2016/07/26 09:51:27, Jakob wrote:
> On 2016/07/26 01:42:49, zhengxing.li wrote:
> > For regression test, the Math.max(-0, 0) test is in result-table-max.js
> already.
> > What's make the bug hard-to-reproduce is the Math.max() works in FC mode
most
> of
> > time.
> > So, It's need to add another Math.max(-0, 0) test?
> 
> In the x87 port you can do whatever you want. If you don't think you need a
> test, go ahead and land without one.
> 
> The general rule of thumb is: if a bug is discovered that the existing test
> suite did not expose, then a test should be added that makes sure that the
> affected code path works correctly. A minimum requirement for such a test is
> that it would have failed before the fix (otherwise, how can you be sure that
> the fix actually fixes the problem, and that the problem won't come back next
> time someone else touches the code?).

Jakob, thank you for your nice comment.

I will consider your comment in future CLs.

[Weiliang, 2016-07-27 06:47:25]

lgtm

[zhengxing.li, 2016-07-27 06:48:04]

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

[commit-bot: I haz the power, 2016-07-27 06:48:09]

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-07-27 07:08:20]

Description was changed from

==========
[X87] [crankshaft] Fix Math.max(-0, 0) bug.

  This CL fixed one bug in crankshaft compiler for Math.max(-0, 0).

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

to

==========
[X87] [crankshaft] Fix Math.max(-0, 0) bug.

  This CL fixed one bug in crankshaft compiler for Math.max(-0, 0).

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

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

Committed patchset #2 (id:20001)

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

Description was changed from

==========
[X87] [crankshaft] Fix Math.max(-0, 0) bug.

  This CL fixed one bug in crankshaft compiler for Math.max(-0, 0).

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

to

==========
[X87] [crankshaft] Fix Math.max(-0, 0) bug.

  This CL fixed one bug in crankshaft compiler for Math.max(-0, 0).

BUG=

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

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

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