Closed
Bug 668299
Opened 14 years ago
Closed 14 years ago
IonMonkey: Remove deallocation band-aid from linear scan register allocator
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: adrake, Assigned: adrake)
References
Details
Attachments
(1 file)
2.81 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
Tracking bug for the associated FIXME in the code. In the event that two intervals start at the same code location and one has a fixed interval, the fixed register may be inadvertently assigned to the interval that does not require it. This band-aid simply re-adds the original interval to the work-list so it can get a new register, but this should be handled by not allocating the required register to the wrong interval in the first place.
Assignee | ||
Comment 1•14 years ago
|
||
The band-aid solution turns out to be the most performant, as the alternative would involve looking ahead to not only other intervals in the queue, but their use chains as well. However, the issue still stands that in the event of an impossible fixed register constraint, the algorithm will not terminate. This patch adds a flag to an interval that identifies that it was selected as a result of a hard fixed-register requirement, and should not ever be deallocated, asserting in the case that this is not possible.
This ensures that all interval-locations may be processed at most twice (note that even this is an extremely uncommon case), restoring the desired termination and linearity properties.
Attachment #543377 -
Flags: review?(dvander)
Comment on attachment 543377 [details] [diff] [review]
Patch v0
Review of attachment 543377 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/ion/RegisterAllocator.cpp
@@ +715,5 @@
> + // and "de-assign" the other one. We can do this
> + // safely (i.e. be guaranteed to terminate) iff
> + // the register we are reallocating was not itself
> + // assigned to fulfill a FIXED constraint.
> + JS_ASSERT(!it->hasFlag(LiveInterval::FIXED));
This flag is only observed in debug mode, so what is this patch changing?
::: js/src/ion/RegisterAllocator.h
@@ +197,4 @@
> return reg_;
> }
>
> + bool hasFlag(Flag flag) {
const
Assignee | ||
Comment 3•14 years ago
|
||
The flag allows the register allocator to assert on nontermination rather than to potentially loop forever. This doesn't change behavior because the behavior from the base patch was correct -- just potentially difficult to debug.
I suspect the flags field will get used for other things, if this turns out not to be the case I will #ifdef DEBUG it.
Assignee | ||
Comment 4•14 years ago
|
||
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 5•14 years ago
|
||
I missed that you didn't r+ this one out of the sea of other r+, will back out if desired.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
![]() |
||
Updated•14 years ago
|
Attachment #543377 -
Flags: review?(dvander) → review+
![]() |
||
Updated•14 years ago
|
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•