function f(y) { return Math.atan2(Math.fround(-Number.MIN_VALUE), ~Math.hypot(y > 0, 5)) } f(-0) print(f(1)) $ ./js-dbg-opt-64-dm-nsprBuild-darwin-6a63bcb6e0d3 --fuzzing-safe --no-threads --ion-eager 4525.js 3.141592653589793 $ ./js-dbg-opt-64-dm-nsprBuild-darwin-6a63bcb6e0d3 --fuzzing-safe --no-threads --ion-eager --ion-check-range-analysis 4525.js -3.141592653589793 Tested this on m-c rev 6a63bcb6e0d3. My configure flags are: CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: https://hg.mozilla.org/mozilla-central/rev/3a545eb9828b user: Brian Hackett date: Tue Aug 26 12:30:36 2014 -0700 summary: Bug 894596 - Bake constant valued object properties into jitcode when possible, r=jandem, patch mostly written by djvj. Brian/Kannan, any idea what's going on here?

This is an older bug that was exposed by bug 894596. The following testcase fails for me with --ion-eager --no-threads on b005b4e38525, which is the parent revision of 3a545eb9828b: with({}){} function f(y) { var a = Math.fround(-0); var b = ~Math.hypot(y > 0, 5); assertEq(a, -0); } f(-0) f(1) I think this is a range analysis bug. We correctly fold Math.fround(-0) to -0, but then the range analysis goes and replaces that value with an int32 0 value in the resume points which use it, and after bailing out after the hypot call we pass that int32 to assertEq instead of -0.

(In reply to Brian Hackett (:bhackett) from comment #1) > I think this is a range analysis bug. We correctly fold Math.fround(-0) to > -0, but then the range analysis goes and replaces that value with an int32 0 > value in the resume points which use it, and after bailing out after the > hypot call we pass that int32 to assertEq instead of -0. Dan, do you happen to know what might be going on here? (It seems to go back beyond end-2013, or https://hg.mozilla.org/mozilla-central/rev/df3c2a1e86d3 )

It is indeed range analysis. Range analysis' current plan for negative zero is to treat it as equal to regular zero, which is consistent with how comparisons work, however this means that isInt32() returns true even when a value could be negative zero, which leads range analysis to think that it can losslessly truncate such a value to int32. Attached is a patch which adds a flag to range analysis to explicitly track whether the range includes negative zero. When this flag is set, isInt32() returns false even if the range is otherwise within the int32 range and there's no fractional part.

Comment on attachment 8501924 [details] [diff] [review] range-negative-zero.patch Review of attachment 8501924 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/RangeAnalysis.h @@ +408,1 @@ > static Range *NewDoubleRange(TempAllocator &alloc, double l, double h) { We should rename these function, so it is not as mysterious as having 2 or 1 argument to distinguish between treating negative zero as zero or negative zero...

Comment on attachment 8501924 [details] [diff] [review] range-negative-zero.patch Review of attachment 8501924 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/ion/bug1073928.js @@ +26,5 @@ > + assertEq(b, -6); > + } > +} > +g(-0, -0); > +g(1, -0); Did you meant "h"? ::: js/src/jit/RangeAnalysis.cpp @@ +334,5 @@ > + if (flags_ & FractionalFlag) { > + if ((flags_ & NegativeZeroFlag) || !canBeZero()) > + sp.printf("F"); > + else > + sp.printf("F(but without -0)"); Appending "U{-0}" at the end of the set would be easier to read. @@ +426,5 @@ > } > > bool newHasInt32LowerBound = lhs->hasInt32LowerBound_ || rhs->hasInt32LowerBound_; > bool newHasInt32UpperBound = lhs->hasInt32UpperBound_ || rhs->hasInt32UpperBound_; > + uint16_t newFlags = lhs->flags_ & rhs->flags_; This is extremely obscure, we should avoid this kind of relations. ::: js/src/jit/RangeAnalysis.h @@ +199,3 @@ > bool hasInt32UpperBound_; > > + uint16_t flags_; I am sorry, but Range Analysis code is so problematic that I prefer us being verbose and clear than manipulating unnamed flags. We should use enumerated values or named bit fields instead of using uint16_t.

Comment on attachment 8501924 [details] [diff] [review] range-negative-zero.patch Review of attachment 8501924 [details] [diff] [review]: ----------------------------------------------------------------- I agree with nbp. We shouldn't have these flag fields, but be verbose and add them as boolean/bit fields. The masks sound like something that will eventually go wrong. Also like mentioned before we should rename the setDouble(d) to be more understandable. This is now very confusing and will cause errors. Something like setDoubleIgnoreNegativeZero() ...

I converted the patch from using a flag word to using individual flag fields. Let me know what you think!

Comment on attachment 8511387 [details] [diff] [review] range-negative-zero.patch Review of attachment 8511387 [details] [diff] [review]: ----------------------------------------------------------------- I am a bit sad that the modification of the fractional part also goes in this patch, but I guess it would be hard to split it at this point. -1, The naming is not yet obvious and this implies that we spend some time to get used to it. +1, Using enum for the arguments provide compiler errors and avoid forgetting about any. Note: should canBeFiniteNegative consider canBeNegativeZero ? ::: js/src/jit/RangeAnalysis.cpp @@ +605,5 @@ > > void > Range::setDouble(double l, double h) > { > + MOZ_ASSERT(!(l > h)); Is this to avoid warning about equality comparisons of doubles? @@ +701,5 @@ > + return new(alloc) Range(l, h, > + ValuesWithFractionalPartsFlag(lhs->canHaveFractionalPart() || > + rhs->canHaveFractionalPart()), > + NegativeZeroFlag(lhs->canBeNegativeZero() || > + rhs->canBeNegativeZero()), This is safe to keep the code as it is, but note that "x + y" can only be negativeZero iff x == -0 and y == -0. @@ +730,5 @@ > + return new(alloc) Range(l, h, > + ValuesWithFractionalPartsFlag(lhs->canHaveFractionalPart() || > + rhs->canHaveFractionalPart()), > + NegativeZeroFlag(lhs->canBeNegativeZero() || > + rhs->canBeNegativeZero()), Same thing with, "x - y" can only be negativeZero iff x == -0 and y == 0. @@ +904,5 @@ > + if ((lhs->lower_ <= 0 && rhs->upper_ >= 0) || > + (rhs->lower_ <= 0 && lhs->upper_ >= 0)) > + { > + newMayIncludeNegativeZero = IncludesNegativeZero; > + } I would prefer if this fits in one relation instead of setting it to one thing and have a correction after, I think the relation should look like: NegativeZeroFlag newMayIncludeNegativeZero = NegativeZeroFlag((lhs->contains(0) && rhs->canBeFiniteNegative()) || (rhs->contains(0) && lhs->canBeFiniteNegative())); fun-facts: Also note that this relation only holds because our range analysis is based on Int32, and not on doubles, otherwise we would have to check with min-exponent which can be computed by the multiplication. (-1e-162 * 1e-162 == -0) @@ +1051,5 @@ > lhs->hasInt32LowerBound_ && rhs->hasInt32LowerBound_, > Min(lhs->upper_, rhs->upper_), > lhs->hasInt32UpperBound_ || rhs->hasInt32UpperBound_, > + ValuesWithFractionalPartsFlag(lhs->mayIncludeValuesWithFractionalParts_ || > + rhs->mayIncludeValuesWithFractionalParts_), nit: indentation issue. @@ +1069,5 @@ > lhs->hasInt32LowerBound_ || rhs->hasInt32LowerBound_, > Max(lhs->upper_, rhs->upper_), > lhs->hasInt32UpperBound_ && rhs->hasInt32UpperBound_, > + ValuesWithFractionalPartsFlag(lhs->mayIncludeValuesWithFractionalParts_ || > + rhs->mayIncludeValuesWithFractionalParts_), nit: same here. @@ +1506,5 @@ > + // negative zero. > + Range::NegativeZeroFlag newMayIncludeNegativeZero = > + Range::NegativeZeroFlag(!lhs.hasInt32LowerBound() || > + lhs.canBeNegativeZero() || > + lhs.lower() < 0); nit: lhs.canBeFiniteNegative() || lhs.canBeNegativeZero() @@ +1530,5 @@ > > // Something simple for now: When dividing by a positive rhs, the result > // won't be further from zero than lhs. > if (lhs.lower() >= 0 && rhs.lower() >= 1) { > + setRange(new(alloc) Range(0, lhs.upper(), nit: trailing white space. @@ +2315,2 @@ > setInt32(0, 1); > + MOZ_ASSERT(isBoolean()); Move this assertion out-side the scope of this "if". ::: js/src/jit/RangeAnalysis.h @@ +201,3 @@ > bool hasInt32UpperBound_; > > + ValuesWithFractionalPartsFlag mayIncludeValuesWithFractionalParts_ : 1; nit: s/IncludeValuesWithFractionalParts/IncludeFractionalPart/ ? @@ +240,2 @@ > mozilla::FloorLog2(mozilla::Abs(upper_))); > + MOZ_ASSERT(max_exponent_ + !!mayIncludeValuesWithFractionalParts_ >= These relations are hard to understand without looking back at the declaration of the enum to understand the coercion which is happening. Either which should be explicit: (mayIncludeValuesWithFractionalParts_ == IncludesValuesWithFractionalParts) which might be too verbose, or we should just make it a boolean, and rename the field to "includesValuesWithFractionalParts_". Note that this does not forbid to keep the enum to distinguish arguments and generate compilation error, as the enum would be coerced to a bool. Also, it would be good to add an assertion that we only include -0, if the range contains 0.

> Note: should canBeFiniteNegative consider canBeNegativeZero ? No, because "negative zero" is not actually negative ;-). But, this function is used by negativeZeroMul, which does need to know about negative zero. I've now added a new function 'canHaveSignBitSet()' which is what negativeZeroMul and some other things actually need. > ::: js/src/jit/RangeAnalysis.cpp > @@ +605,5 @@ > > > > void > > Range::setDouble(double l, double h) > > { > > + MOZ_ASSERT(!(l > h)); > > Is this to avoid warning about equality comparisons of doubles? No, it's just a basic sanity check. It would be MOZ_ASSERT(l <= h) except that we don't want it to fail if l or h is NaN. > @@ +701,5 @@ > > + return new(alloc) Range(l, h, > > + ValuesWithFractionalPartsFlag(lhs->canHaveFractionalPart() || > > + rhs->canHaveFractionalPart()), > > + NegativeZeroFlag(lhs->canBeNegativeZero() || > > + rhs->canBeNegativeZero()), > > This is safe to keep the code as it is, but note that "x + y" can only be > negativeZero iff x == -0 and y == -0. Oh, neat. That's easy enough to do. > @@ +730,5 @@ > > + return new(alloc) Range(l, h, > > + ValuesWithFractionalPartsFlag(lhs->canHaveFractionalPart() || > > + rhs->canHaveFractionalPart()), > > + NegativeZeroFlag(lhs->canBeNegativeZero() || > > + rhs->canBeNegativeZero()), > > Same thing with, "x - y" can only be negativeZero iff x == -0 and y == 0. Also done now. > @@ +904,5 @@ > > + if ((lhs->lower_ <= 0 && rhs->upper_ >= 0) || > > + (rhs->lower_ <= 0 && lhs->upper_ >= 0)) > > + { > > + newMayIncludeNegativeZero = IncludesNegativeZero; > > + } > > I would prefer if this fits in one relation instead of setting it to one > thing and have a correction after, I think the relation should look like: > > NegativeZeroFlag newMayIncludeNegativeZero = > NegativeZeroFlag((lhs->contains(0) && rhs->canBeFiniteNegative()) || > (rhs->contains(0) && lhs->canBeFiniteNegative())); Actually, I think we can do even better. We don't need to test contains(0) because the optimize() method will clear the negative zero flag if the resulting range excludes zero. And this can also use the canHaveSignBitSet() utility. > fun-facts: Also note that this relation only holds because our range > analysis is based on Int32, and not on doubles, otherwise we would have to > check with min-exponent which can be computed by the multiplication. > (-1e-162 * 1e-162 == -0) Using canHaveSignBitSet() and relying on optimize() to handle the case where zero is excluded, the relation no longer depends on this. > @@ +1506,5 @@ > > + // negative zero. > > + Range::NegativeZeroFlag newMayIncludeNegativeZero = > > + Range::NegativeZeroFlag(!lhs.hasInt32LowerBound() || > > + lhs.canBeNegativeZero() || > > + lhs.lower() < 0); > > nit: lhs.canBeFiniteNegative() || lhs.canBeNegativeZero() This is another use case for the canHaveSignBitSet() function. > ::: js/src/jit/RangeAnalysis.h > @@ +201,3 @@ > > bool hasInt32UpperBound_; > > > > + ValuesWithFractionalPartsFlag mayIncludeValuesWithFractionalParts_ : 1; > > nit: s/IncludeValuesWithFractionalParts/IncludeFractionalPart/ ? Ok. > @@ +240,2 @@ > > mozilla::FloorLog2(mozilla::Abs(upper_))); > > + MOZ_ASSERT(max_exponent_ + !!mayIncludeValuesWithFractionalParts_ >= > > These relations are hard to understand without looking back at the > declaration of the enum to understand the coercion which is happening. > > Either which should be explicit: > (mayIncludeValuesWithFractionalParts_ == > IncludesValuesWithFractionalParts) > > which might be too verbose, or we should just make it a boolean, and rename > the field to "includesValuesWithFractionalParts_". Note that this does not > forbid to keep the enum to distinguish arguments and generate compilation > error, as the enum would be coerced to a bool. The enum implicitly converts to bool, and this is actually used in several other places in this patch, so I think what I really need to do here is just restructure the code a little. What do you think of the code in the new patch? > Also, it would be good to add an assertion that we only include -0, if the > range contains 0. We currently handle this in optimize(). That seemed to be simpler than making it an invariant, and having all the code which computes ranges perform this check manually, but it could be done.

Comment on attachment 8513919 [details] [diff] [review] range-negative-zero.patch Review of attachment 8513919 [details] [diff] [review]: ----------------------------------------------------------------- I would be ok with this patch if we strip the "may" from the fields/variable names. Does that sounds good for you? This really confused me when I read the previous patch the last time, now I just ignored it while reviewing this patch and understood the patch better than the previous one. ::: js/src/jit/RangeAnalysis.cpp @@ +635,5 @@ > uint16_t hExp = ExponentImpliedByDouble(h); > max_exponent_ = Max(lExp, hExp); > > + mayIncludeFractionalParts_ = IncludesFractionalParts; > + mayIncludeNegativeZero_ = IncludesNegativeZero; Did you meant exclude here, knowing that the conditions below are explicitly testing for ranges before setting the Include flag. @@ +699,5 @@ > e = Range::IncludesInfinityAndNaN; > > + return new(alloc) Range(l, h, > + FractionalPartsFlag(lhs->canHaveFractionalPart() || > + rhs->canHaveFractionalPart()), nit: over-indent issue? @@ +728,5 @@ > e = Range::IncludesInfinityAndNaN; > > + return new(alloc) Range(l, h, > + FractionalPartsFlag(lhs->canHaveFractionalPart() || > + rhs->canHaveFractionalPart()), nit: same here.

Does "may" here mean "is allowed to" or "may or may not"? (I'm guessing that ambiguity is why it's confusing)

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #11) > Does "may" here mean "is allowed to" or "may or may not"? (I'm guessing that > ambiguity is why it's confusing) The problem here is that the field is used to describe a property of the Range, so the range may have the property or not. Thus the property name should not be named "mayInclude…" as it does not give hints on the boolean value of the enumerated value, but "include…" (or "exclude…", but not in this precise case).

Comment on attachment 8513919 [details] [diff] [review] range-negative-zero.patch Review of attachment 8513919 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay. Overall question: 1) can you remove the new on every "newMayIncludeXXXX" variable name? 2) I also used to see 'can' instead of 'may'. So can we use 'can' instead? ::: js/src/jit/RangeAnalysis.cpp @@ +457,5 @@ > bool newHasInt32UpperBound = lhs->hasInt32UpperBound_ || rhs->hasInt32UpperBound_; > + > + FractionalPartsFlag newMayIncludeFractionalParts = > + FractionalPartsFlag(lhs->mayIncludeFractionalParts_ && > + rhs->mayIncludeFractionalParts_); I think everything after the = fits on one line. If I calculated correct 97 columnno @@ +460,5 @@ > + FractionalPartsFlag(lhs->mayIncludeFractionalParts_ && > + rhs->mayIncludeFractionalParts_); > + NegativeZeroFlag newMayIncludeNegativeZero = > + NegativeZeroFlag(lhs->mayIncludeNegativeZero_ && > + rhs->mayIncludeNegativeZero_); Please put everything after the = on one line. @@ +524,5 @@ > bool newHasInt32UpperBound = hasInt32UpperBound_ && other->hasInt32UpperBound_; > + > + FractionalPartsFlag newMayIncludeFractionalParts = > + FractionalPartsFlag(mayIncludeFractionalParts_ || > + other->mayIncludeFractionalParts_); Please put everything after the = on one line. @@ +527,5 @@ > + FractionalPartsFlag(mayIncludeFractionalParts_ || > + other->mayIncludeFractionalParts_); > + NegativeZeroFlag newMayIncludeNegativeZero = > + NegativeZeroFlag(mayIncludeNegativeZero_ || > + other->mayIncludeNegativeZero_); Please put everything after the = on one line. @@ +613,5 @@ > lower_ = int32_t(::floor(l)); > hasInt32LowerBound_ = true; > + } else if (l >= INT32_MAX) { > + lower_ = INT32_MAX; > + hasInt32LowerBound_ = true; Is this related to the fix? It seems like this is an improvement, not related to the bug found here? Since range analysis is already hard enough, can you open a new bug for this? @@ +623,5 @@ > upper_ = int32_t(::ceil(h)); > hasInt32UpperBound_ = true; > + } else if (h <= INT32_MIN) { > + upper_ = INT32_MIN; > + hasInt32UpperBound_ = true; Same. @@ +635,5 @@ > uint16_t hExp = ExponentImpliedByDouble(h); > max_exponent_ = Max(lExp, hExp); > > + mayIncludeFractionalParts_ = IncludesFractionalParts; > + mayIncludeNegativeZero_ = IncludesNegativeZero; Yeah it doesn't make sense to Include here. And have this calculation beneath ;) @@ +702,5 @@ > + FractionalPartsFlag(lhs->canHaveFractionalPart() || > + rhs->canHaveFractionalPart()), > + NegativeZeroFlag(lhs->canBeNegativeZero() && > + rhs->canBeNegativeZero()), > + e); Also the e); is not on the correct indentation. @@ +731,5 @@ > + FractionalPartsFlag(lhs->canHaveFractionalPart() || > + rhs->canHaveFractionalPart()), > + NegativeZeroFlag(lhs->canBeNegativeZero() && > + rhs->canBeZero()), > + e); Also the e); is not on the correct indentation. @@ +894,5 @@ > Range::mul(TempAllocator &alloc, const Range *lhs, const Range *rhs) > { > + FractionalPartsFlag newMayIncludeFractionalParts = > + FractionalPartsFlag(lhs->mayIncludeFractionalParts_ || > + rhs->mayIncludeFractionalParts_); Please try to fit everything after the = on one line. @@ +1019,5 @@ > { > int32_t l = op->lower_; > int32_t u = op->upper_; > + FractionalPartsFlag mayIncludeFractionalParts = > + op->mayIncludeFractionalParts_; This should fit on one line. @@ +1042,5 @@ > return nullptr; > > + FractionalPartsFlag newMayIncludeFractionalParts = > + FractionalPartsFlag(lhs->mayIncludeFractionalParts_ || > + rhs->mayIncludeFractionalParts_); plz put on same line after = @@ +1045,5 @@ > + FractionalPartsFlag(lhs->mayIncludeFractionalParts_ || > + rhs->mayIncludeFractionalParts_); > + NegativeZeroFlag newMayIncludeNegativeZero = > + NegativeZeroFlag(lhs->mayIncludeNegativeZero_ || > + rhs->mayIncludeNegativeZero_); plz put on same line after = @@ +1065,5 @@ > return nullptr; > > + FractionalPartsFlag newMayIncludeFractionalParts = > + FractionalPartsFlag(lhs->mayIncludeFractionalParts_ || > + rhs->mayIncludeFractionalParts_); plz put on same line after = @@ +1068,5 @@ > + FractionalPartsFlag(lhs->mayIncludeFractionalParts_ || > + rhs->mayIncludeFractionalParts_); > + NegativeZeroFlag newMayIncludeNegativeZero = > + NegativeZeroFlag(lhs->mayIncludeNegativeZero_ || > + rhs->mayIncludeNegativeZero_); plz put on same line after = ::: js/src/jit/RangeAnalysis.h @@ +630,5 @@ > max_exponent_ = exponentImpliedByInt32Bounds(); > assertInvariants(); > } > > + // Set this rangel to include values >= l and <= h. Note that this s/rangel/range/ @@ +639,5 @@ > + // Set this range to the narrowest possible range containing d. > + // This function treats negative zero as distinct from zero, since this > + // makes the narrowest possible range containin zero a range which > + // contains one value rather than two. > + void setDoubleSingleton(double d); Thanks

(In reply to Hannes Verschore [:h4writer] from comment #13) > Comment on attachment 8513919 [details] [diff] [review] > range-negative-zero.patch > > Review of attachment 8513919 [details] [diff] [review]: > ----------------------------------------------------------------- > > Sorry for the delay. > > Overall question: > 1) can you remove the new on every "newMayIncludeXXXX" variable name? The new helps avoid confusion inside of functions like unionWith and intersect, so that we don't have both canHaveFractionalPart and canHaveFractionalPart_ to keep track of. > 2) I also used to see 'can' instead of 'may'. So can we use 'can' instead? Changed, and other comments addressed. One other bug came up; in Range::setDouble, the code to check negative zero by testing for (l <= 0 && h >= 0) wasn't setting the negative zero flag when either l or h is NaN. I fixed this by changing it to (!(l > 0) && !(h < 0)). https://hg.mozilla.org/integration/mozilla-inbound/rev/f718ec4b4cb0