643733 - TI+JM: Assertion failure: Call site vanished., at ../methodjit/Compiler.cpp:2443

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

var x; 
-(typeof (x+x))
OR
-(typeof Math.abs())

The problem is that we call a stub for JSOP_NEG, then we recompile and this time typeof pushes a constant string and JSOP_NEG does constant folding. 

The easiest fix is probably to make JSOP_NEG only constant fold numbers.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 520916 [details] [diff] [review]
Patch

Hm not sure if this is entirely correct, will check some other things first.

Comment 3

[Jan de Mooij [:jandem]]

Another option is to add some constraint to jsop_typeofexpr to recompile after Math.abs(), but I'm not sure if we need it.

Comment 4

[Brian Hackett [Laid off!]]

I think the problem is in jsop_typeof.  To allow constant propagation/folding to happen when recompiling without having to worry about emitting rejoin paths, the constants in the recompiled code should be a subset of those in the original code (we used to want them to be strictly equal, but that has been relaxed).  However, in the recompiled code the fe's known types may be better or worse than in the original code, so the compiler should not rely only on the known types of entries to do constant folding.  jsop_typeof could instead alloc a register and move the string value into it, pushing a known string but not a constant.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Patch

This makes more sense indeed.

Comment 6

[Brian Hackett [Laid off!]]

http://hg.mozilla.org/projects/jaegermonkey/rev/7a76d795f62c

Comment 7

[Brian Hackett [Laid off!]]

According to AWFY this might have regressed v8-crypto, which uses typeof in several places (though not I think on terribly hot paths).

Comment 8

[Jan de Mooij [:jandem]]

(In reply to comment #7)
> According to AWFY this might have regressed v8-crypto, which uses typeof in
> several places (though not I think on terribly hot paths).

Reverting the patch does not affect performance much using the SS-harness or v8-v6 from the V8 repository. I can't reproduce the v8-crypto regression at all at revision af6d3ea1e6e7 using opt32 build with -m -n, can you reproduce it locally?

Comment 9

[Brian Hackett [Laid off!]]

Hmm, thanks for looking.  I haven't tried to repro, but unless an obvious issue presents itself we can just let this sit until I start on LICM (after scripted call inlining is done), that will be focusing on v8-crypto and kraken.

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Patch 2

It should be safe now to push a constant value in jsop_typeof. Most of the other bugs like this are not perf-critical but I'd like to have this back for jsop_typeof. Especially with TI enabled this allows us to better optimize some typeof expressions.

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Patch 2

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/releases/mozilla-release/rev/eab484f82ca4
[INFER] Push constant value in jsop_typeof, bug 643733. r=bhackett