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

RESOLVED FIXED in mozilla24

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jruderman, Assigned: luke)

Tracking

(Blocks 1 bug, {testcase})

Trunk
mozilla24
x86_64
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

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

print(1 / negZeroMinusNegZero()());

asm.js gives -Infinity
normal gives Infinity
Assignee

Comment 1

6 years ago
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)
Assignee

Updated

6 years ago
No longer blocks: odinfuzz
Assignee

Comment 2

6 years ago
Posted patch fix and test (obsolete) — Splinter Review
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #760075 - Flags: review?(jdemooij)
Assignee

Updated

6 years ago
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.  :-)
Assignee

Comment 4

6 years ago
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: 6 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.