Last Comment Bug 715460 - IonMonkey: Assertion failure: interval->start() < pos && pos < interval->end() on ARM
: IonMonkey: Assertion failure: interval->start() < pos && pos < interval->end(...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: ARM Linux
: -- normal (vote)
: ---
Assigned To: Jan de Mooij [:jandem]
:
:
Mentors:
Depends on: 712278
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-05 02:40 PST by Marty Rosenberg [:mjrosenb]
Modified: 2012-01-12 02:01 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (14.37 KB, patch)
2012-01-07 10:15 PST, Jan de Mooij [:jandem]
marty.rosenberg: review+
Details | Diff | Splinter Review
Patch v2 (15.66 KB, patch)
2012-01-09 01:34 PST, Jan de Mooij [:jandem]
dvander: review+
Details | Diff | Splinter Review

Description Marty Rosenberg [:mjrosenb] 2012-01-05 02:40:46 PST
TEST-UNEXPECTED-FAIL | jit_test.py --ion-eager    | /home/mrosenberg/src/ionmonkey/ionmonkey-build/js/src/jit-test/tests/jaeger/bug639459.js: Assertion failure: interval->start() < pos && pos < interval->end(), at /home/mrosenberg/src/ionmonkey/ionmonkey-build/js/src/ion/LinearScan.cpp:1153

backtrace is:
#3  0x0027e3dc in JS_Assert (s=0x6279d4 "interval->start() < pos && pos < interval->end()", file=0x6272a4 "/home/mrosenberg/src/ionmonkey/ionmonkey-build/js/src/ion/LinearScan.cpp", ln=1153)
    at /home/mrosenberg/src/ionmonkey/ionmonkey-build/js/src/jsutil.cpp:115
#4  0x004a9618 in js::ion::LinearScanAllocator::splitInterval (this=0xbedef600, interval=0x72cad0, pos=...) at /home/mrosenberg/src/ionmonkey/ionmonkey-build/js/src/ion/LinearScan.cpp:1153
#5  0x004a9a0c in js::ion::LinearScanAllocator::assign (this=0xbedef600, allocation=...) at /home/mrosenberg/src/ionmonkey/ionmonkey-build/js/src/ion/LinearScan.cpp:1207
#6  0x004aabf8 in js::ion::LinearScanAllocator::allocateRegisters (this=0xbedef600) at /home/mrosenberg/src/ionmonkey/ionmonkey-build/js/src/ion/LinearScan.cpp:720
#7  0x004ae274 in js::ion::LinearScanAllocator::go (this=0xbedef600) at /home/mrosenberg/src/ionmonkey/ionmonkey-build/js/src/ion/LinearScan.cpp:1742
#8  0x0045bd74 in TestCompiler (builder=..., graph=...) at /home/mrosenberg/src/ionmonkey/ionmonkey-build/js/src/ion/Ion.cpp:646
#9  0x0045c37c in IonCompile (cx=0x7226e0, script=0x40b06388, fp=0x4054c078, osrPc=0x0) at /home/mrosenberg/src/ionmonkey/ionmonkey-build/js/src/ion/Ion.cpp:698
#10 0x0045c67c in js::ion::Compile (cx=0x7226e0, script=0x40b06388, fp=0x4054c078, osrPc=0x0) at /home/mrosenberg/src/ionmonkey/ionmonkey-build/js/src/ion/Ion.cpp:823
#11 0x0016691c in js::Interpret (cx=0x7226e0, entryFrame=0x4054c020, interpMode=js::JSINTERP_NORMAL) at /home/mrosenberg/src/ionmonkey/ionmonkey-build/js/src/jsinterp.cpp:3452
#12 0x001739e0 in js::RunScript (cx=0x7226e0, script=0x40b06420, fp=0x4054c020) at /home/mrosenberg/src/ionmonkey/ionmonkey-build/js/src/jsinterp.cpp:487
#13 0x00173d04 in js::ExecuteKernel (cx=0x7226e0, script=0x40b06420, scopeChain=..., thisv=..., type=js::EXECUTE_GLOBAL, evalInFrame=0x0, result=0x0) at /home/mrosenberg/src/ionmonkey/ionmonkey-build/js/src/jsinterp.cpp:684
#14 0x00173ff8 in js::Execute (cx=0x7226e0, script=0x40b06420, scopeChainArg=..., rval=0x0) at /home/mrosenberg/src/ionmonkey/ionmonkey-build/js/src/jsinterp.cpp:725
#15 0x00043558 in JS_ExecuteScript (cx=0x7226e0, obj=0x40b03040, script=0x40b06420, rval=0x0) at /home/mrosenberg/src/ionmonkey/ionmonkey-build/js/src/jsapi.cpp:5280
#16 0x0001980c in Process (cx=0x7226e0, obj=0x40b03040, filename=0xbedf135d "/home/mrosenberg/src/ionmonkey/ionmonkey-build/js/src/jit-test/tests/jaeger/bug639459.js", forceTTY=false)
    at /home/mrosenberg/src/ionmonkey/ionmonkey-build/js/src/shell/js.cpp:484
#17 0x0001a868 in ProcessArgs (cx=0x7226e0, obj=0x40b03040, op=0xbedf0f28) at /home/mrosenberg/src/ionmonkey/ionmonkey-build/js/src/shell/js.cpp:5173
#18 0x0001ad5c in Shell (cx=0x7226e0, op=0xbedf0f28, envp=0xbedf1138) at /home/mrosenberg/src/ionmonkey/ionmonkey-build/js/src/shell/js.cpp:5270
#19 0x0001b894 in main (argc=8, argv=0xbedf1114, envp=0xbedf1138) at /home/mrosenberg/src/ionmonkey/ionmonkey-build/js/src/shell/js.cpp:5510
Comment 1 Jan de Mooij [:jandem] 2012-01-06 03:36:17 PST
The problem is this line:

    return a / a;

ARM wants a fixed register for both lhs and rhs:

    LDivI *lir = new LDivI(useFixed(div->lhs(), r0), useFixed(div->rhs(), r1),
                           tempFixed(r2), tempFixed(r3)/*, tempFixed(lr)*/);

So what happens is that we want "a" in both r0 and r1 and this fails. I'm not sure yet what's the best way to handle this - I guess we used to add two overlapping intervals to the same VirtualRegister but I like the invariant that intervals for a vreg don't overlap (at least I think we have that invariant atm and would like to assert it somewhere).
Comment 2 Jan de Mooij [:jandem] 2012-01-07 10:15:36 PST
Created attachment 586709 [details] [diff] [review]
Patch

Having useFixed(lhs) and useFixed(rhs) seems reasonable and I think the register allocators should support the case where lhs and rhs are the same virtual register.

However, I don't think this (very rare) case justifies the refactoring and added complexity we'd need for bug 712278. So the patch special-cases this by inserting a move from the first fixed register to the second one.

It also fixes the ARM implementation of LDivI so that the C call no longer clobbers r0 and r1. We now pass the failing test and the test I added with both register allocators.

Marty, can you review the ARM bits?
Comment 3 Marty Rosenberg [:mjrosenb] 2012-01-07 22:42:09 PST
Comment on attachment 586709 [details] [diff] [review]
Patch

Review of attachment 586709 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ion/arm/Lowering-arm.cpp
@@ +275,5 @@
>  bool
>  LIRGeneratorARM::lowerDivI(MDiv *div)
>  {
> +    LDivI *lir = new LDivI(useFixedAtStart(div->lhs(), r0), useFixedAtStart(div->rhs(), r1),
> +                           tempFixed(r1), tempFixed(r2), tempFixed(r3)/*, tempFixed(lr)*/);

Do we want to have r1 in both useFixedAtStart, and tempFixed?
Comment 4 Jan de Mooij [:jandem] 2012-01-09 01:32:30 PST
(In reply to Marty Rosenberg [:Marty] from comment #3)
> Do we want to have r1 in both useFixedAtStart, and tempFixed?

Yes, the use ensures that rhs is in r1, and the temp is used so that the C call can clobber it. I'll add a comment here.
Comment 5 Jan de Mooij [:jandem] 2012-01-09 01:34:12 PST
Created attachment 586927 [details] [diff] [review]
Patch v2

Fix a (potential) LSRA bug.
Comment 6 David Anderson [:dvander] 2012-01-10 11:08:28 PST
Comment on attachment 586927 [details] [diff] [review]
Patch v2

Review of attachment 586927 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ion/LinearScan.cpp
@@ +448,5 @@
> + */
> +bool
> +LinearScanAllocator::copyFixedRegister(LInstruction *ins, CodePosition pos, LUse *src, LUse *dest)
> +{
> +    AnyRegister srcReg = GetFixedRegister(vregs[src].def(), src);

Can we assert in here that this is never used with a nunbox or gcthing?
Comment 7 Jan de Mooij [:jandem] 2012-01-12 02:01:37 PST
(In reply to David Anderson [:dvander] from comment #6)
> Can we assert in here that this is never used with a nunbox or gcthing?

Pushed with these asserts.

https://hg.mozilla.org/projects/ionmonkey/rev/dac569ab767c

Note You need to log in before you can comment on or make changes to this bug.