Closed Bug 643733 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

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.
Assignee: general → jandemooij
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
Attachment #520916 - Flags: review?(bhackett1024)
Comment on attachment 520916 [details] [diff] [review]
Patch

Hm not sure if this is entirely correct, will check some other things first.
Attachment #520916 - Flags: review?(bhackett1024)
Another option is to add some constraint to jsop_typeofexpr to recompile after Math.abs(), but I'm not sure if we need it.
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.
Attached patch PatchSplinter Review
This makes more sense indeed.
Attachment #520916 - Attachment is obsolete: true
Attachment #520991 - Flags: review?(bhackett1024)
Attachment #520991 - Flags: review?(bhackett1024) → review+
http://hg.mozilla.org/projects/jaegermonkey/rev/7a76d795f62c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
According to AWFY this might have regressed v8-crypto, which uses typeof in several places (though not I think on terribly hot paths).
(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?
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.
Attached patch Patch 2 (obsolete) — Splinter Review
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.
Attachment #527278 - Flags: review?(bhackett1024)
Attached patch Patch 2Splinter Review
Attachment #527278 - Attachment is obsolete: true
Attachment #527278 - Flags: review?(bhackett1024)
Attachment #527279 - Flags: review?(bhackett1024)
Attachment #527279 - Attachment mime type: application/octet-stream → text/plain
Attachment #527279 - Attachment is patch: true
Attachment #527279 - Flags: review?(bhackett1024) → review+
You need to log in before you can comment on or make changes to this bug.