IonMonkey: Remove deallocation band-aid from linear scan register allocator

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: adrake, Assigned: adrake)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

7 years ago
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)

Updated

7 years ago
Assignee: general → adrake
Blocks: 657816
(Assignee)

Comment 1

7 years ago
Created attachment 543377 [details] [diff] [review]
Patch v0

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

7 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

7 years ago
http://hg.mozilla.org/users/danderson_mozilla.com/ionmonkey/rev/d21af975756a
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Comment 5

7 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 → ---
Attachment #543377 - Flags: review?(dvander) → review+
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.