Closed Bug 715460 Opened 12 years ago Closed 12 years ago

IonMonkey: Assertion failure: interval->start() < pos && pos < interval->end() on ARM

Categories

(Core :: JavaScript Engine, defect)

ARM
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: mjrosenb, Assigned: jandem)

References

Details

Attachments

(1 file, 1 obsolete file)

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
Assignee: general → jdemooij
Status: NEW → ASSIGNED
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).
Depends on: 712278
Attached patch Patch (obsolete) — Splinter Review
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?
Attachment #586709 - Flags: review?(mrosenberg)
Attachment #586709 - Flags: review?(dvander)
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?
Attachment #586709 - Flags: review?(mrosenberg) → review+
(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.
Attached patch Patch v2Splinter Review
Fix a (potential) LSRA bug.
Attachment #586709 - Attachment is obsolete: true
Attachment #586709 - Flags: review?(dvander)
Attachment #586927 - Flags: review?(dvander)
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?
Attachment #586927 - Flags: review?(dvander) → review+
(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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.