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

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).

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 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?

(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.

Fix a (potential) LSRA bug.

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?

(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