1246200 - Differential Testing: Different output message involving truncation error

Status: RESOLVED DUPLICATE of bug 1236114

(Core :: JavaScript Engine: JIT, defect)

Description

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

g = print
g
g = function() {};
g = function() {};
g = function() {};
g = function() {};
g = function() {};
g = function() {};
(function() {
    f = (function(y) {
        return !~y % (1 / 0 >> Number.MAX_VALUE) !== g()
    });
    f()
    f()
})()
gc();
g = function() {
    return ([] << [])
};
print(f())


$ ./js-dbg-64-dm-clang-darwin-5e024441510f --fuzzing-safe --no-threads --baseline-eager testcase.js
true

$ ./js-dbg-64-dm-clang-darwin-5e024441510f --fuzzing-safe --no-threads --ion-eager testcase.js
false

Tested this on m-c rev 5e024441510f.

My configure flags are:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin14.5.0 --disable-jemalloc --enable-debug --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/funfuzz/js/compileShell.py -b "--enable-debug --enable-more-deterministic" -r 5e024441510f


autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/d791ba00bf06
user:        Hannes Verschore
date:        Fri Aug 14 12:45:47 2015 +0200
summary:     Bug 1171945: IonMonkey: Use tryXXX structure for jsop_binary in IonBuilder, r=jandem

Hannes, is bug 1171945 a likely regressor?

Comment 1

[Hannes Verschore [:h4writer]]

I think we have a structural problem with recover instructions as is.

Small example:
return 0 % (1 / 0 >> 0) !== 0

MDiv 1 0 (int32)
MRsh div 0 (int32)
MMod 0 rsh
MCompare mod inf
MReturn compare

during GVN we do:
(note: "0 % (1 / 0 >> 0)" == NaN and 0 cannot by NaN)

MDiv 1 0 (int32)
MRsh div 0 (int32)
MMod 0 rsh
MReturn true

during trucation we do:
(note: '-' means on the recover path)

MDiv 1 0 (int32 t)
- MRsh div 0 (int32)
MMod 0 div
MReturn true

during sinking/dce we do:
- MDiv 1 0 (int32 t)
- MRsh div 0 (int32)
- MMod 0 div
MReturn true

Which all seems find. But now the catch.
The result is fine, but the recover instructions are wrong!

- MDiv 1 0 (int32 t)
- MRsh div 0 (int32)
- MMod 0 div
MBail (mod ....)
MReturn true

If we want to recover between MMod and MCompare,
we captured the mod as recover instruction.
During recovering we do:
1/0 = Infinity
0 % Infinity = 0
As a result the MMod will be '0', but it should be 'NaN'.

That is because we recover an optimized path.
MDiv has been optimized to return "MDiv | 0" as answer,
while the recover instruction does a plain div.
=> Okay that sounds easy enough to add.


What prompted me to think this is a more general case:
Imagine we have an instruction that has a bailout path.
We assume the result of that instruction is Int32 and
make this assumption throughout the rest of the code.
Eventually during Sinking we DCE all that code, inclusive that instruction.
Though for some reason we bailout and need to recover that instruction and next.
Note: That instruction would have taken the bailout path, if it wasn't recovered

BUT: What do we do in recover instructions?
1) (not currently) All our recover instructions are based on the most general form of the bytecode and handle all cases. As a result we recover that instruction and forward it into the next recover instruction. Since all recover instructions are general and handle all types this shouldn't be a problem

2) (not currently) The recover instruction tries to mimick what happens in ion code. It executes the code during recovering, and hits the edge case. We saved the resume point at that position and bail again, but now at the resume point of that instruction. (warning: I think this is impossible)

3) (currently) We execute the recover instruction in all globality and don't care which result we get out of it. We don't care if the next recover instruction is only correct for certain types/numbers. We just assume everything will go correctly. As a result a "MDiv 1 0" can be typed as Int32, the next few instructions can assume only integers will flow in their input and improve their code. When we go to the recover path, we will see Infinity and just forward that in the "optimized based on Int32 type" next recover instruction. 

4) (not currently) This is an approach I have discussed before, which would really hurt this system a lot. We could mark all functions that bailout as not-recoverable. That means they wouldn't be eliminated and suddenly make a type change during recovering. This means that as soon as a type change happens ion exits and we don't have to find a solution on what to do when this case happens during recovering.

I think we need to discuss this more / better / in-depth and come with a strategy on how to handle / fix this.
Doesn't seem like a quick and easy definitely good fix is for grab?

Comment 2

[Hannes Verschore [:h4writer]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #0)
> The first bad revision is:
> changeset:   https://hg.mozilla.org/mozilla-central/rev/d791ba00bf06
> user:        Hannes Verschore
> date:        Fri Aug 14 12:45:47 2015 +0200
> summary:     Bug 1171945: IonMonkey: Use tryXXX structure for jsop_binary in
> IonBuilder, r=jandem
> 
> Hannes, is bug 1171945 a likely regressor?

The code in there changes some things which would result in this case being pampered over. The testcase wouldn't reproduce, but it wouldn't fix the real issue that I described above solve.

Comment 3

[Nicolas B. Pierron [:nbp]]

Yes, the problem is in the fact that Range Analysis truncate phase has an extra phases which remove bit-ops which make a destructive optimization by removing useless bitops.  On the other hand Sinking assume that none of the data flow should changed, and that any operations should remain as-if they were manipulating double.

The bit-ops removal was made mainly to optimize asm.js memory accesses to reduce the complexity of the pattern matches in the Effective Address Analysis (transformation).

I have a patch to fix this issue in Bug 1236114.  This patch split the truncate phase in 2, and move the Sink phase after the Truncate & UCE phase, but before the bit-ops removal and the Effective Address Analysis phase.

Comment 4

[Hannes Verschore [:h4writer]]

Talked about this issue with nbp on irc and agreeing this would be solved fully with comment 3.

Seems like nbp wanted to maintain variant (1), but still allow transformation, only if they are not 'destructive'.
Where I don't have a good definition what 'destructive' means and we need to define better. But transformation are allowed as long as the resulting MIR instruction does the same as the original set of instructions it is replacing !FOR every possibly value!.

Proposed solution to still allow 'destructive' transformation and only capture the recover instructions later or in multiple passes:
(5) Nexto keeping a list of operands, also keep a pointer to the list of recover operands. Whenever we do a destructive transformation, we need to update the operands to still point to the actual operands. When we put instructions into the recover path, we will recover the 'list of recover operands' instead of the current operands. That way we maintain that the recover instructions operate on the not-adjusted graph or a graph that is similar to the actual graph.

Comment 6

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

(In reply to Hannes Verschore [:h4writer] from comment #4)
> Talked about this issue with nbp on irc and agreeing this would be solved
> fully with comment 3.

Hannes, are you or :nbp likely going to take this then?

Comment 7

[Nicolas B. Pierron [:nbp]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #6)
> (In reply to Hannes Verschore [:h4writer] from comment #4)
> > Talked about this issue with nbp on irc and agreeing this would be solved
> > fully with comment 3.
> 
> Hannes, are you or :nbp likely going to take this then?

As Hannes reopened this bug, I assumed he was going to take over this bug.

On the other hand, I recall hearing from Eric that we should proceed with Bug 1236114 for the moment, and keep this bug open for further investigation, (Eric, is that correct?)  but I am not sure this was the correct message as the discussion was mixing this issue with Bug 1239075.

Comment 8

[Hannes Verschore [:h4writer]]

There has been some discussion on who will take this on. Since nbp has other goals, I have volunteered. Therefore assigning to me.

Comment 9

[Hannes Verschore [:h4writer]]

This is fixed by bug 1236114

Comment 10

[Eric Faust [:efaust]]

Duplicate. Updating status flags.