Fix and improve unreachable parsing#8617
Open
tlively wants to merge 1 commit intosimplify-unreachable-printfrom
Open
Fix and improve unreachable parsing#8617tlively wants to merge 1 commit intosimplify-unreachable-printfrom
tlively wants to merge 1 commit intosimplify-unreachable-printfrom
Conversation
The recent change (#8608) that reduced the number of scratch locals introduced by IRBuilder also inadvertently introduced a bug. That commit changed the conditions under which we package multiple expressions into a block to be popped together. We previously did this whenever there was a get of a scratch local to hoist a value to the top of the stack. After #8608, we started using blocks whenever there were multiple expressions hoisted. These conditions are usually identical, but we missed the case where there are multiple expressions and also no `get` because the value is unreachable. The introduction of a block where there was none before invalidated our logic for determining whether or not to pop back past an unreachable expression based on whether the expressions underneath it would satisfy the type constraints of the parent. Simplify that logic by calculating the stack size at which we must avoid popping past an unreachable rather than calculating the index of the unreachable itself. This makes the logic work correctly whether or not the unreachable will be popped as part of a block. That in turn lets us remove an extra unreachable we were conservatively pushing onto the stack when "hoisting" unreachable values. Add tests for the cases where we can and cannot pop past an unreachable that is followed by a none-typed expression. The former case previously parsed incorrectly and the latter case is now parsed without introducing extra unreachable instructions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The recent change (#8608) that reduced the number of scratch locals introduced by IRBuilder also inadvertently introduced a bug. That commit changed the conditions under which we package multiple expressions into a block to be popped together. We previously did this whenever there was a get of a scratch local to hoist a value to the top of the stack. After #8608, we started using blocks whenever there were multiple expressions hoisted. These conditions are usually identical, but we missed the case where there are multiple expressions and also no
getbecause the value is unreachable. The introduction of a block where there was none before invalidated our logic for determining whether or not to pop back past an unreachable expression based on whether the expressions underneath it would satisfy the type constraints of the parent.Simplify that logic by calculating the stack size at which we must avoid popping past an unreachable rather than calculating the index of the unreachable itself. This makes the logic work correctly whether or not the unreachable will be popped as part of a block. That in turn lets us remove an extra unreachable we were conservatively pushing onto the stack when "hoisting" unreachable values.
Add tests for the cases where we can and cannot pop past an unreachable that is followed by a none-typed expression. The former case previously parsed incorrectly and the latter case is now parsed without introducing extra unreachable instructions.