Tweak minus zero detect of HChange

Tweak minus zero detect of HChange

Eliminate some useless minus zero check

BUG=

[Jakob Kummerow, 2013-11-12 09:02:02]

Nope.

var double_one = Math.cos(0);

function add(a, b) {
  return a + b;
}

assertEquals(1, 1/add(double_one, 0));
assertEquals(1, 1/add(0, double_one));
%OptimizeFunctionOnNextCall(add);
assertEquals(1/(-0 + -0), 1/add(-0, -0));

[Yang, 2013-11-12 10:14:56]

On 2013/11/12 09:02:02, Jakob wrote:
> Nope.
> 
> var double_one = Math.cos(0);
> 
> function add(a, b) {
>   return a + b;
> }
> 
> assertEquals(1, 1/add(double_one, 0));
> assertEquals(1, 1/add(0, double_one));
> %OptimizeFunctionOnNextCall(add);
> assertEquals(1/(-0 + -0), 1/add(-0, -0));

I suggest adding both tests to minus-zero.js to increase coverage and prevent
future mistakes.

[Weiliang, 2013-11-12 15:32:10]

On 2013/11/12 10:14:56, Yang wrote:
> On 2013/11/12 09:02:02, Jakob wrote:
> > Nope.
> > 
> > var double_one = Math.cos(0);
> > 
> > function add(a, b) {
> >   return a + b;
> > }
> > 
> > assertEquals(1, 1/add(double_one, 0));
> > assertEquals(1, 1/add(0, double_one));
> > %OptimizeFunctionOnNextCall(add);
> > assertEquals(1/(-0 + -0), 1/add(-0, -0));
> 
> I suggest adding both tests to minus-zero.js to increase coverage and prevent
> future mistakes.

Seems the optimization only applys to SMI. so the logic may change to below

  if (representation().IsSmi() || type().IsSmi()) {
    result->set_can_be_minus_zero(false);
  } else {
    result->set_can_be_minus_zero(!to().IsInteger32() ||
                                  !CheckFlag(kAllUsesTruncatingToInt32));
  }

Is is reasonable?

[Jakob Kummerow, 2013-11-12 16:12:35]

On 2013/11/12 15:32:10, Weiliang wrote:
> On 2013/11/12 10:14:56, Yang wrote:
> > On 2013/11/12 09:02:02, Jakob wrote:
> > > Nope.
> > > 
> > > var double_one = Math.cos(0);
> > > 
> > > function add(a, b) {
> > >   return a + b;
> > > }
> > > 
> > > assertEquals(1, 1/add(double_one, 0));
> > > assertEquals(1, 1/add(0, double_one));
> > > %OptimizeFunctionOnNextCall(add);
> > > assertEquals(1/(-0 + -0), 1/add(-0, -0));
> > 
> > I suggest adding both tests to minus-zero.js to increase coverage and
prevent
> > future mistakes.
> 
> Seems the optimization only applys to SMI. so the logic may change to below
> 
>   if (representation().IsSmi() || type().IsSmi()) {
>     result->set_can_be_minus_zero(false);
>   } else {
>     result->set_can_be_minus_zero(!to().IsInteger32() ||
>                                   !CheckFlag(kAllUsesTruncatingToInt32));
>   }
> 
> Is is reasonable?

There is no fundamental difference between Smi and Integer32 representation
here: both can't explicitly store -0, but might be used for values that have an
actual value of -0, and must hence check for cases where the difference is
observable. 

I'm not sure whether it's possible right now to create a Smi-mode instruction
where the inputs don't go through a whatever-to-smi change that would deopt on
inputs that would later create a -0 result, but either way basing further
optimizations on this happenstance would be way too brittle. (And as CL author
it's your job to convince the reviewer that the change in question can't
possibly go wrong, not the other way round!)

Types are different in that they actually provide some guarantees about ranges
themselves. However, the current HType system is both inconsistent and
deprecated. I would be highly reluctant to apply any micro-optimizations to it
at this point -- the next big thing in this area will be consistent use of the
new "Type" classes we've recently introduced. I think they will fundamentally
change the whole Range inference system.

[Weiliang, 2013-11-13 06:42:40]

On 2013/11/12 16:12:35, Jakob wrote:
> On 2013/11/12 15:32:10, Weiliang wrote:
> > On 2013/11/12 10:14:56, Yang wrote:
> > > On 2013/11/12 09:02:02, Jakob wrote:
> > > > Nope.
> > > > 
> > > > var double_one = Math.cos(0);
> > > > 
> > > > function add(a, b) {
> > > >   return a + b;
> > > > }
> > > > 
> > > > assertEquals(1, 1/add(double_one, 0));
> > > > assertEquals(1, 1/add(0, double_one));
> > > > %OptimizeFunctionOnNextCall(add);
> > > > assertEquals(1/(-0 + -0), 1/add(-0, -0));
> > > 
> > > I suggest adding both tests to minus-zero.js to increase coverage and
> prevent
> > > future mistakes.
> > 
> > Seems the optimization only applys to SMI. so the logic may change to below
> > 
> >   if (representation().IsSmi() || type().IsSmi()) {
> >     result->set_can_be_minus_zero(false);
> >   } else {
> >     result->set_can_be_minus_zero(!to().IsInteger32() ||
> >                                   !CheckFlag(kAllUsesTruncatingToInt32));
> >   }
> > 
> > Is is reasonable?
> 
> There is no fundamental difference between Smi and Integer32 representation
> here: both can't explicitly store -0, but might be used for values that have
an
> actual value of -0, and must hence check for cases where the difference is
> observable. 
> 
> I'm not sure whether it's possible right now to create a Smi-mode instruction
> where the inputs don't go through a whatever-to-smi change that would deopt on
> inputs that would later create a -0 result, but either way basing further
> optimizations on this happenstance would be way too brittle. (And as CL author
> it's your job to convince the reviewer that the change in question can't
> possibly go wrong, not the other way round!)
> 
> Types are different in that they actually provide some guarantees about ranges
> themselves. However, the current HType system is both inconsistent and
> deprecated. I would be highly reluctant to apply any micro-optimizations to it
> at this point -- the next big thing in this area will be consistent use of the
> new "Type" classes we've recently introduced. I think they will fundamentally
> change the whole Range inference system.

Sorry, Just a quick thought before going to bed. 

And after second thought today, I upload the patch set2 and also include your
testcases. Could you please have a review. Thanks!

[Weiliang, 2013-11-15 03:21:49]

rebase

[Jakob Kummerow, 2013-11-18 14:06:15]

As I said before, there's no fundamental difference between Smi and Integer
representation when it comes to -0: Both can't encode it directly, so we need
(or at least currently have) a flag indicating that the real value could indeed
have been -0, and in situations where the difference becomes observable must be
checked accordingly.
Just because Lithium code generation currently happens to make it hard
(impossible? not sure...) to write a test that would fail with this patch
doesn't mean that baking such assumptions into Hydrogen phases is acceptable.

To put it bluntly: I don't think this patch is a good idea.