Closed Bug 875656 Opened 7 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: gkw, Assigned: sunfish)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase, Whiteboard: [fuzzblocker])

Attachments

(2 files, 1 obsolete file)

Attached file 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
Flags: needinfo?(bhackett1024)
Summary: Assertion failure: !minimalInterval(interval), at ion/BacktrackingAllocator.cpp → Assertion failure: !minimalInterval(interval), at ion/BacktrackingAllocator.cpp with enableSPSProfilingAssertions
Brian, re-ping? This still reproduces as of m-c rev 3b955f306226.
Flags: needinfo?(kvijayan)
I'm not sure why I'm marked with needInfo on this.  Is it because of the involvement of SPS enabling?
(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.
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.
Flags: needinfo?(kvijayan)
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.
Flags: needinfo?(bhackett1024)
Attached patch regalloc-concat-pressure.patch (obsolete) — Splinter Review
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.
Assignee: general → sunfish
Attachment #808864 - Flags: review?(bhackett1024)
Attachment #808864 - Flags: review?(bhackett1024) → review+
https://hg.mozilla.org/mozilla-central/rev/8bb615862099
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Duplicate of this bug: 868731
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/64222cba97fa
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Oops, re-reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
Blocks: 826741
The useFixedAtStart utility introduced in bug 923799 may provide a way to fix the patch above.
Component: JavaScript Engine → JavaScript Engine: JIT
Target Milestone: mozilla27 → ---
This patch is similar to the previous patch, but uses useFixedAtStart to properly describe clobbering the lhs and rhs input registers.
Attachment #808864 - Attachment is obsolete: true
Attachment #821894 - Flags: review?(bhackett1024)
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.
Attachment #821894 - Flags: review?(bhackett1024) → review+
(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
https://hg.mozilla.org/mozilla-central/rev/f030f97fcf10
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.