30.20 KB, text/plain
823 bytes, patch
|Details | Diff | Splinter Review|
419 bytes, text/plain
1.19 KB, patch
|Details | Diff | Splinter Review|
Created attachment 499221 [details] LIR listing of the problem code. Found in: swfcontents/y/Q/q/e/Y/YqQeYqtmpL2mtsnSJo2efZRkdzTX+Z3E2mCgrtuZgUI= I don't know what the problem is yet, but I want to get the bug entered anyway so we don't lose it. Other than being some random assert, a few interesting things appear in the LIR listing: 1. the function name is mx.utils::UIDUtil$/isUID(). So this is our code, not some random thing off the web. 2. the LIR listing has calls to multiple variants of stringCharCodeAt() with the same args: 81: String_charCodeAtFU1 = calld.none #String_charCodeAtFU ( ldq10 ldi2 ) 131: String_charCodeAtIU1 = calli.none #String_charCodeAtIU ( ldq10 ldi2 ) There looks like quite a bit of branchy control flow, too.
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → Future
Attachment #499221 - Attachment mime type: application/octet-stream → text/plain
As a data point, x86 fails with a different assertion: Assertion failure: (_allocator.active[REGNUM(FST0)] && _fpuStkDepth == -1) || (!_allocator.active[REGNUM(FST0)] && _fpuStkDepth == 0) (/Users/edwin/vm/jitrefac/nanojit/Assembler.cpp:371)
this bug is a possible duplicate of bug 614510; not marking it yet until it is investigated.
I can't find file swfcontents/y/Q/q/e/Y/YqQeYqtmpL2mtsnSJo2efZRkdzTX+Z3E2mCgrtuZgUI=. Can you run with ' -Dverbose=jit,verify,raw,regs ' before and after applying patches from bug 607816. You should be able to see something similar to what's described in comment 3 in bug 614510 if this is indeed the same problem. And btw the issues manifests as a 'frame entry wasn't freed' assertion, not an fpu assertion: Eg. frame entry xxx wasn't freed : _entries[i] == __null
Moving from Spicy to Wasabi for investigation.
Target Milestone: flash10.2.x-Spicy → flash10.x-Wasabi
Most likely dup of bug 609028
Moving to Anza
This should already be fixed, it just need to be validated against the test case outlined in the bug desc.
Created attachment 547586 [details] [diff] [review] Bad source Here's a simple script that will demo the problem: rebuild it and run using a Debug Debugger build. If you run 'normally' (jit or interp) it works; if you run with -Dnodebugger, it asserts, and if continued after the assert, incorrectly prints "false" instead of "true".
Comment on attachment 547586 [details] [diff] [review] Bad source Note also that a Release build will (incorrectly) print false unless you force -Dinterp.
Created attachment 547735 [details] Even simpler source Even simpler testcase that demonstrates the problem. Note that if you change "var c:Number" into "var c:uint", the problem goes away.
Digging further, it appears that the specialization in optimizeIntCmpWithNumberCall() is questionable in this case (though it's not clear whether/why it's causing the misbehavior)... we have already called charCodeAt(), and since the result is stored into an explicit Number type, we specialize to the "FU" variant returning float. However, in test vs literal '45', we note that it's an integer literal compared against charCodeAt result, and emit another call to the "IU" variant returning int. Yikes, certainly not a win to make redundant calls.
OK, so the problem is that the FU->IU specialization occurs in both branches, but in the 'else' branch, we reuse the same LIns... but that's only valid in the 'if' branch since that's the call site. -- Note 1: it's a little disturbing that the LIns-recycling-code can emit stale stuff this way. -- Note 2: even if it didn't recycle, we'd still have potentially suboptimal code since we'd be emitting two charCodeAt() calls where one would suffice.
Aha! The gotcha here is in specializeIntCall(); it attempts to avoid redundant calls by keeping a hashmap of specialized calls it's emitted, but this doesn't consider that they may have been emitted in code paths that are mutually exclusive...
Created attachment 547786 [details] [diff] [review] Patch Here's a simplistic fix that seems to address the issue: clear the specialized-function map when we insert a label. This is suboptimal, in that we may well be inserting redundant calls in some cases, but it should eliminate incorrect usage. It's not clear to me if there's an existing structure in the JIT that allows us to do a smarter job of hoisting potentially redundant pure calls in this way -- is there?
(In reply to comment #15) > Review ping? This is a bit conservative, as you noted: If one specialization occurs above a control-flow diamond, and another identical one below, we'll duplicate the call even though the first dominates the second. Unfortunately, we don't have the machinery to do better than this, so I think the patch is a reasonable way to restore correctness. Aside from this bug, the specializer has the weakness that we may have already been forced to emit an unspecialized call anyway by the time we come to a use that allows for a context-dependent specialization. At runtime, we may end of calling the both unspecialized version and a specialization, when it might be cheaper just to coerce the result of the unspecialized call. This specializer is really on shaky ground, and probably looked good on benchmarks only because the screw cases are rare in practice, or perhaps just in the few benchmarks measured. I think it may be advisable to re-run the benchmarks upon which this optimization was originally justified, and make sure we're still seeing a net win.
Comment on attachment 547586 [details] [diff] [review] Bad source yep, pretty bad source! great test case.
Attachment #547586 - Flags: feedback?(wmaddox) → feedback+
Comment on attachment 547786 [details] [diff] [review] Patch Patch looks find contingent on test cases and no serious regressions, as Bill mentioned.
Attachment #547786 - Flags: superreview?(edwsmith) → superreview+
> I think it may be advisable to re-run the benchmarks upon which this > optimization was originally justified, and make sure we're still seeing a > net win. Do we know which benchmarks those were? This needs to land in Serrano, which is scheduled for ZBB by end of week, so there's some urgency here.
the main ones have charCodeAt in their name (under test/acceptance/jsmicro and asmicro), but looking for new outliers would be good too.
(In reply to comment #20) > the main ones have charCodeAt in their name (under test/acceptance/jsmicro > and asmicro), but looking for new outliers would be good too. Good call. I'll go ahead and run before-and-after benchmarks on those, at least.
before-and-after on mac-x86 and mac-x64 don't show any red flags. I'm going to land it if/when William gives the R+.
Comment on attachment 547786 [details] [diff] [review] Patch My concerns were only with respect to possible performance regression. From a correctness viewpoint, it looks sound. R+
pushed to TR: 6490:8de850f0d909
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
pushed to TR-Serrano: 6340:f3b64bc72d18
You need to log in before you can comment on or make changes to this bug.