998580 - Differential Testing: Different output message involving bitwise zero and empty with block

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

function g(f, x) {
    for (var j = 0; j < 2; ++j) {
        for (var k = 0; k < 3; ++k) {
            print(f(x[j], x[1]))
        }
    }
}
with({}) {}
(function() {
    function f(x) {
        return (-0x80000000 == -(x | 0))
    }
    g(f, [0xf, 0x80000000])
})()


$ ./js-opt-64-dm-ts-darwin-582b2d81ebe1 --fuzzing-safe --ion-parallel-compile=off 300.js
false
false
false
false
false
false

$ ./js-opt-64-dm-ts-darwin-582b2d81ebe1 --fuzzing-safe --ion-parallel-compile=off --ion-eager 300.js
false
false
false
true
true
true

(Tested this on 64-bit Mac js opt threadsafe deterministic shell off m-c rev 582b2d81ebe1)

My configure flags (Mac) 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-optimize --disable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --disable-tests --enable-more-deterministic --with-ccache --enable-threadsafe <other NSPR options>

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/9658dbcf4cd7
user:        Dan Gohman
date:        Mon Dec 09 07:11:12 2013 -0800
summary:     Bug 943303 - IonMonkey: Convert floating-point comparisons to integer using range analysis. r=nbp

Dan, is bug 943303 a likely regressor?

Comment 1

[Dan Gohman [:sunfish]]

It's likely.

Comment 3

[Dan Gohman [:sunfish]]

Attachment: fix-kinds.patch

This patch generalizes range analysis truncation to also support a new form of conversion to integer which still requires full bailout checks. This allows us to more precisely describe what happens when we convert an MCompare to an integer. It doesn't silently truncate its operands, but it still wants them to have integer types if possible.

While here, this patch also incorporates indirect truncation as well, eliminating the code that did an extra use-list traversal.

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Here's another testcase that is fixed by the patch here:

function f(x) {
    return 0 > -0 + -~x
}
f(0)
print(f(2147483647))


$ ./js-opt-64-dm-ts-darwin-3b166b8add93 --fuzzing-safe --ion-parallel-compile=off 10508.js
false

$ ./js-opt-64-dm-ts-darwin-3b166b8add93 --fuzzing-safe --ion-parallel-compile=off --ion-eager 10508.js
true

Comment 5

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8411443 [details] [diff] [review]
fix-kinds.patch

Review of attachment 8411443 [details] [diff] [review]:
-----------------------------------------------------------------

I am not sure to understand why we need this renaming of fix -> truncate?
"Fix" sounds like this it was originally an error and that we have to do it.  "Truncate" is more descriptive and sounds optional.

Note, this patch entirely disable the truncation of comparison operands, as comparisons are lowering the "fix" (truncation level) to a level which is never used for truncating anything.

::: js/src/jit/MIR.h
@@ +421,5 @@
> +        FixWithBailouts,
> +        // The value will be truncated after some arithmetic (see above).
> +        IndirectTruncate,
> +        // Direct and infallible truncation to int32.
> +        Truncate

nit: As you are using the Min/Max functions on the FixKind, I'll suggest that we provide a numbering, even if it is implicit, and comment that we use it to merge truncation kinds.

@@ +3532,5 @@
>          return AliasSet::None();
>      }
>  
>      bool isTruncated() const {
> +        return implicitFix_ == Truncate;

Sadly, even if this code is correct for MDiv, it also de-optimize other arithmetic operations.
A simple test case such as:

function f(x) {
  var a = x | 0;
  return (a + a + a + a) | 0;
}

Should not use doubles.

MAdd::fallible() function should check that the operation is at least implicitly truncated, instead of checking isTruncated.

::: js/src/jit/RangeAnalysis.cpp
@@ +2167,5 @@
>  
>      if (type() == MIRType_Double || type() == MIRType_Int32) {
>          specialization_ = MIRType_Int32;
>          setResultType(MIRType_Int32);
> +        if (kind == Truncate && range())

nit: kind >= IndirectTruncate

@@ +2184,5 @@
>  
>      if (type() == MIRType_Double || type() == MIRType_Int32) {
>          specialization_ = MIRType_Int32;
>          setResultType(MIRType_Int32);
> +        if (kind == Truncate && range())

nit: kind >= IndirectTruncate

@@ +2201,5 @@
>  
>      if (type() == MIRType_Double || type() == MIRType_Int32) {
>          specialization_ = MIRType_Int32;
>          setResultType(MIRType_Int32);
> +        if (kind == Truncate) {

nit: kind >= IndirectTruncate

@@ +2265,4 @@
>      // We use the return type to flag that this MToDouble should be replaced by
>      // a MTruncateToInt32 when modifying the graph.
>      setResultType(MIRType_Int32);
> +    if (kind == Truncate) {

nit: kind >= IndirectTruncate

Comment 6

[Dan Gohman [:sunfish]]

Attachment: truncate-kinds.patch

(In reply to Nicolas B. Pierron [:nbp] from comment #5)
> Comment on attachment 8411443 [details] [diff] [review]
> fix-kinds.patch
> 
> Review of attachment 8411443 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I am not sure to understand why we need this renaming of fix -> truncate?
> "Fix" sounds like this it was originally an error and that we have to do it.
> "Truncate" is more descriptive and sounds optional.

I was trying to avoid ambiguity with "truncate" meaning either "full truncate" or something other kind of truncate. I reverted this now.

> Note, this patch entirely disable the truncation of comparison operands, as
> comparisons are lowering the "fix" (truncation level) to a level which is
> never used for truncating anything.

This is as intended. Comparison isn't a full-truncating operation. Before this patch, truncation mainly because it was the only way to get operands into integer registers. This patch introduces a way to get the operands into integer registers without actually truncating them.

> ::: js/src/jit/MIR.h
> @@ +421,5 @@
> > +        FixWithBailouts,
> > +        // The value will be truncated after some arithmetic (see above).
> > +        IndirectTruncate,
> > +        // Direct and infallible truncation to int32.
> > +        Truncate
> 
> nit: As you are using the Min/Max functions on the FixKind, I'll suggest
> that we provide a numbering, even if it is implicit, and comment that we use
> it to merge truncation kinds.

Ok.

> @@ +3532,5 @@
> >          return AliasSet::None();
> >      }
> >  
> >      bool isTruncated() const {
> > +        return implicitFix_ == Truncate;
> 
> Sadly, even if this code is correct for MDiv, it also de-optimize other
> arithmetic operations.
> A simple test case such as:
> 
> function f(x) {
>   var a = x | 0;
>   return (a + a + a + a) | 0;
> }
> 
> Should not use doubles.
> 
> MAdd::fallible() function should check that the operation is at least
> implicitly truncated, instead of checking isTruncated.

Thanks for spotting that. I fixed this, and your simple testcase is once again codegen'd without overflow checks.

> ::: js/src/jit/RangeAnalysis.cpp
> @@ +2167,5 @@
> >  
> >      if (type() == MIRType_Double || type() == MIRType_Int32) {
> >          specialization_ = MIRType_Int32;
> >          setResultType(MIRType_Int32);
> > +        if (kind == Truncate && range())
> 
> nit: kind >= IndirectTruncate

That's pretty subtle, but I think it's right.

Comment 7

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8415665 [details] [diff] [review]
truncate-kinds.patch

Review of attachment 8415665 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/MIR.h
@@ +421,5 @@
> +    //   3. Ignore negative zeros. (-0 --> 0)
> +    //   4. Ignore remainder. (3 / 4 --> 0)
> +    //
> +    // Indirect truncation is used to represent that we are interested in the
> +    // truncated result, but only if they it can safely flow in operations which

self-nit: … if >they< it can …

@@ +2972,5 @@
>  #endif
> +
> +    TruncateKind truncateKind() const {
> +        return implicitTruncate_; 
> +    } 

style-nit: fix trailing white-spaces.

Comment 8

[Dan Gohman [:sunfish]]

(In reply to Nicolas B. Pierron [:nbp] from comment #7)
> self-nit: … if >they< it can …
> style-nit: fix trailing white-spaces.

Fixed and fixed. https://hg.mozilla.org/integration/mozilla-inbound/rev/695049af7654

Comment 9

[Dan Gohman [:sunfish]]

The patch had the wrong bug number. The following patches revert it and reapply it with the correct number:
http://hg.mozilla.org/integration/mozilla-inbound/rev/87feb441d562
http://hg.mozilla.org/integration/mozilla-inbound/rev/75413bfa9dd8

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/87feb441d562

Comment 11

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/23d16c2f0f67

Backed out for causing:

Bug 1006301
Bug 1006561
Bug 1006850

(Ref bug 1006301 comment 5)

Comment 12

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Backout made it to m-c:

https://hg.mozilla.org/mozilla-central/rev/23d16c2f0f67

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b66e279688a1

Comment 14

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Carsten Book [:Tomcat] from comment #13)
> https://hg.mozilla.org/mozilla-central/rev/b66e279688a1

This landed as part of bug 1006301.