878429 - IonMonkey: Different result for (-0.0 - -0.0)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

function negZeroMinusNegZero()
{
  "use asm";
  function f()
  {
    return +(-0.0 - -0.0);
  }
  return f;
}

print(1 / negZeroMinusNegZero()());

asm.js gives -Infinity
normal gives Infinity

Comment 1

[Luke Wagner [:luke]]

Ah hah, this seems to be an underlying Ion bug as illustrated by this testcase (with --ion-eager):

function negZeroMinusNegZero()
{
  var x = -0.0;
  var y = -0.0;
  return +(x - y);
}

print(1 / negZeroMinusNegZero());
print(1 / negZeroMinusNegZero());

> Infinity
> -Infinity

(Ion is only saved from the test-case in comment 0 by *parse-time* constant folding.)

Comment 2

[Luke Wagner [:luke]]

Attachment: fix and test

Comment 3

[Jeff Walden [:Waldo]]

I saw this bug and was surprised the natural implementation wouldn't just work here, so the report of this being an Ion bug got me interested enough to step and debug some.  :-)  ...and then I collided with the patch/test, but the fix looks not the right one to me -- mozilla::detail::DoublePun, just say no! -- so I might as well make it anyway.

MDefinition *
MBinaryArithInstruction::foldsTo(bool useValueNumbers)
{
    ...
    if (MDefinition *folded = EvaluateConstantOperands(this))
        return folded;

    // 0 + -0 = 0. So we can't remove addition
    if (isAdd() && specialization_ != MIRType_Int32)
        return this;

    if (IsConstant(rhs, getIdentity()))
        return lhs;

EvaluateConstantOperands returns NULL because the subtraction folds to Int32Value(0), which is a type change from MIRType_Double.  I don't understand why that makes any sense, that the folded value has to have the same type as the incoming constants, but I'll pretend this makes any sense to me because it probably makes sense to someone.  :-)  Maybe it's some types-are-fixed-by-IonBuilder restriction or something?

isAdd() is obviously false.

I'm hazarding a guess that getIdentity() (+0 for MSub) is supposed to be the binaryarith numeric value that will, for that operation, produce the left-hand side (this meaning should be commented!).  Execution says the RHS (-0) == +0, so the lhs (-0) is (wrongly) returned.

This makes me think isAdd() there should be (isAdd() || isSub()), with the comment updated to include -0 - -0 = +0, as the spot-fixiest fix.  But I remain confused as to why constants can only be folded if they have the exact same type as the un-folded MIR node, so I will merely point this out, not provide a patch, and clamor for education.  :-)

Comment 4

[Luke Wagner [:luke]]

Attachment: fix and 2 tests

It would appear that double subtraction *does* have an identity since
  -0 - +0 = -0
so I'm sticking by my original patch unless Waldo proves that SM (interp) and V8 are wrong ;)

Comment 5

[Jeff Walden [:Waldo]]

http://logbot.glob.com.au/?c=mozilla%23jsapi#c195101 and the next 40-odd minutes (at least) have me/us concluding that +0 is an identity for subtraction, and correspondingly -0 is an identity for addition.  (!)

I would be image-macroing up http://www.byrnerobotics.com/forum/uploads/BradBrickley/2010-11-30_085914_first-peanuts.gif with IEEE-754 right now and submitting it to mozillamemes, but somewhat sadly I'm too fastidious about copyright (for an image that isn't a classic image-macro, at least) to actually make/submit it.  :-\

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> EvaluateConstantOperands returns NULL because the subtraction folds to
> Int32Value(0), which is a type change from MIRType_Double.  I don't
> understand why that makes any sense, that the folded value has to have the
> same type as the incoming constants, but I'll pretend this makes any sense
> to me because it probably makes sense to someone.  :-)  Maybe it's some
> types-are-fixed-by-IonBuilder restriction or something?

EvaluateConstantOperands is used by GVN and GVN runs after the type analysis phase. Type analysis is responsible for adding conversion instructions (MBox, MToDouble, etc.) so that an instruction's operands have the right type.

For instance, in this case if we have something like:

  0.5 + (x - y)

And EvaluateConstantOperands folds x - y (double) to an int32-constant, the MAdd will have an int32-operand even though it's specialization is Double and this will likely confuse Lowering.

Comment 7

[Jeff Walden [:Waldo]]

So if EvaluateConstantOperands were slightly smarter about preserving double results as double values, there would be no need to not constant-fold here, correct?

Comment 8

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e30d56cf29d

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8e30d56cf29d

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7)
> So if EvaluateConstantOperands were slightly smarter about preserving double
> results as double values, there would be no need to not constant-fold here,
> correct?

Correct. If folding returns an int32 value but we expect a double we could just use a DoubleValue..