Closed Bug 668299 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: adrake, Assigned: adrake)

References

Details

Attachments

(1 file)

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: general → adrake
Blocks: 657816
Attached patch Patch v0Splinter Review
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
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.
http://hg.mozilla.org/users/danderson_mozilla.com/ionmonkey/rev/d21af975756a
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
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
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.