[lsra]: Fix nested live ranges due to finishLoop
#313
Conversation
In certain edge cases, `finishLoop` would not insert `LsraEnd`s for active variables resulting in nested live ranges which manifested as an infinite loop in the register allocator (see discussion here [1]). This commit fixes this bug by keeping track of whether a variable will be made active in the future or not (i.e. if it will have a live range). If a variable will have a live range in the future, then `finishLoop` will make sure to insert `LsraEnd`s for them to avoid the above bug. [1]: titzer#308
|
@titzer Feel free to edit this PR directly if you think it's a good baseline. The hardest part for me was to reason about I'm not entirely sure what I have is correct because surely this is not the only test/program which has variables whose live range ends inside a loop and then are used in the future as well. I worry this change has artificially increased the live range for certain cases, but I don't know how I would have checked that. What I mean is when we remove an |
|
I noticed that splitting critical edges before codegen fixes all of these issues. I am planning on doing a stable rev of the compiler and have basically been whacking bugs and making things more stable. Thus a simpler and less invasive change is attractive here. |
|
Okay sure. I think LSRA still needs debug assertions to prevent this from slipping through. Feel free to close this PR since you've merged your fix. |
In certain edge cases,
finishLoopwould not insertLsraEnds for active variables resulting in nested live ranges which manifested as an infinite loop in the register allocator (see discussion here 1).This commit fixes this bug by keeping track of whether a variable will be made active in the future or not (i.e. if it will have a live range). If a variable will have a live range in the future, then
finishLoopwill make sure to insertLsraEnds for them to avoid the above bug.