875656 - Assertion failure: !minimalInterval(interval), at ion/BacktrackingAllocator.cpp with enableSPSProfilingAssertions

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

enableSPSProfilingAssertions(false);
Object.getOwnPropertyNames(this);

asserts js debug shell on m-c changeset 53bfd38cbc8c with --ion-eager --ion-regalloc=backtracking at Assertion failure: !minimalInterval(interval), at ion/BacktrackingAllocator.cpp

This blocks fuzzing --ion-regalloc=backtracking with enableSPSProfilingAssertions

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Brian, re-ping? This still reproduces as of m-c rev 3b955f306226.

Comment 2

[Kannan Vijayan [:djvj]]

I'm not sure why I'm marked with needInfo on this.  Is it because of the involvement of SPS enabling?

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Kannan Vijayan [:djvj] from comment #2)
> I'm not sure why I'm marked with needInfo on this.  Is it because of the
> involvement of SPS enabling?

Yep, I don't know if it's SPS or the backtracking allocator at play here.

Comment 4

[Kannan Vijayan [:djvj]]

I'm pretty sure it's the backtracking allocator that's causing the problem here.  SPS doesn't interact in any way with the compiler backend.

Comment 5

[Dan Gohman [:sunfish]]

It appears SPS does interact with the backend in one way: it makes the FramePointer unavailable to the register allocator.

x86 and x64 both have CallTempReg6 defined to rbp/ebp, which is the FramePointer. On x64 it might be possible to define use a different register, but x86 there are no other registers. With the %ebp unavailable, x86 only has 6 allocatable registers left, which is not enough for the 7 CallTempReg variables.

Comment 6

[Dan Gohman [:sunfish]]

Attachment: regalloc-concat-pressure.patch

The only thing using all 7 CallTemp registers is Concat. This patch tweaks Concat's code slightly to reduce the number of registers used to 6. This reduces register pressure in general, and it also fixes this bug.

Comment 7

[Dan Gohman [:sunfish]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8bb615862099

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8bb615862099

Comment 10

[Dan Gohman [:sunfish]]

Backed out here:
https://hg.mozilla.org/integration/mozilla-inbound/rev/64222cba97fa

Reusing lhs and rhs as temporaries is not valid, because they are only declared to the register allocator as input operands. I'll need to find a different way to fix this.

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/64222cba97fa

Comment 12

[Dan Gohman [:sunfish]]

Oops, re-reopening.

Comment 13

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Dan Gohman [:sunfish] from comment #10)
> Backed out here:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/64222cba97fa
> 
> Reusing lhs and rhs as temporaries is not valid, because they are only
> declared to the register allocator as input operands. I'll need to find a
> different way to fix this.

This patch may also have caused bug 921263.

Comment 14

[Dan Gohman [:sunfish]]

The useFixedAtStart utility introduced in bug 923799 may provide a way to fix the patch above.

Comment 15

[Dan Gohman [:sunfish]]

Attachment: regalloc-concat-pressure.patch

This patch is similar to the previous patch, but uses useFixedAtStart to properly describe clobbering the lhs and rhs input registers.

Comment 16

[Brian Hackett [Laid off!]]

Comment on attachment 821894 [details] [diff] [review]
regalloc-concat-pressure.patch

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

::: js/src/jit/Lowering.cpp
@@ +1479,2 @@
>                                       useFixed(lhs, CallTempReg0),
>                                       useFixed(rhs, CallTempReg1),

I think these two should be useFixedAtStart.

Comment 17

[Dan Gohman [:sunfish]]

(In reply to Brian Hackett (:bhackett) from comment #16)
> Comment on attachment 821894 [details] [diff] [review]
> regalloc-concat-pressure.patch
> 
> Review of attachment 821894 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/Lowering.cpp
> @@ +1479,2 @@
> >                                       useFixed(lhs, CallTempReg0),
> >                                       useFixed(rhs, CallTempReg1),
> 
> I think these two should be useFixedAtStart.

Yes, fixed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f030f97fcf10

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f030f97fcf10