Closed Bug 998580 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect)

All
macOS
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: gkw, Assigned: sunfish)

References

(Blocks 1 open bug)

Details

(Keywords: regression, testcase)

Attachments

(1 file, 1 obsolete file)

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?
Flags: needinfo?(sunfish)
It's likely.
Assignee: nobody → sunfish
Flags: needinfo?(sunfish)
Duplicate of this bug: 999857
Attached patch fix-kinds.patch (obsolete) — Splinter Review
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.
Attachment #8411443 - Flags: review?(nicolas.b.pierron)
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 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
Attachment #8411443 - Flags: review?(nicolas.b.pierron)
(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.
Attachment #8411443 - Attachment is obsolete: true
Attachment #8415665 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8415665 - Flags: review?(nicolas.b.pierron) → review+
(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
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
https://hg.mozilla.org/mozilla-central/rev/87feb441d562
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Depends on: 1006561
Backout made it to m-c:

https://hg.mozilla.org/mozilla-central/rev/23d16c2f0f67
Target Milestone: mozilla32 → ---
https://hg.mozilla.org/mozilla-central/rev/b66e279688a1
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
(In reply to Carsten Book [:Tomcat] from comment #13)
> https://hg.mozilla.org/mozilla-central/rev/b66e279688a1

This landed as part of bug 1006301.
Depends on: 1054972
Depends on: 1061428
You need to log in before you can comment on or make changes to this bug.