Closed Bug 878429 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86_64
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: jruderman, Assigned: luke)

Details

(Keywords: testcase)

Attachments

(1 file, 1 obsolete file)

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

print(1 / negZeroMinusNegZero()());

asm.js gives -Infinity
normal gives Infinity
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.)
Summary: OdinMonkey: Different result for (-0.0 - -0.0) → IonMonkey: Different result for (-0.0 - -0.0)
No longer blocks: odinfuzz
Attached patch fix and test (obsolete) — Splinter Review
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #760075 - Flags: review?(jdemooij)
Attachment #760075 - Attachment is patch: true
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.  :-)
Attached patch fix and 2 testsSplinter Review
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 ;)
Attachment #760075 - Attachment is obsolete: true
Attachment #760075 - Flags: review?(jdemooij)
Attachment #760082 - Flags: review?(jdemooij)
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.  :-\
Attachment #760082 - Flags: review?(jdemooij) → review+
(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.
So if EvaluateConstantOperands were slightly smarter about preserving double results as double values, there would be no need to not constant-fold here, correct?
https://hg.mozilla.org/mozilla-central/rev/8e30d56cf29d
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
(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..
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: