Conversation
When an instruction has a type immediate that we cannot print because the expression it is supposed to come from has unreachable or null type, we instead print an unreachable block with a comment saying what instruction we failed to print. We previously handled this via different code paths for type immediates that come from child expressions that type immediates that come from the printed expression's own return type. Unify these code paths in the printer by improving `Properties::hasUnwritableTypeImmediate` to handle both cases. Also fix the printing of `ShallowExpression` to check for unwritable type immediates first to avoid assertion failures.
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.
kripken
reviewed
Apr 20, 2026
| if (!cast->desc->type.isRef()) { | ||
| return true; | ||
| } | ||
| if (!cast->desc->type.getHeapType().getDescribedType()) { |
Member
Author
There was a problem hiding this comment.
Ah, this is from an outdated version of #8616. Will update this branch.
But to answer the question, yes, this would fail validation, but this code is used from the Printer, so needs to work correctly for invalid IR as well.
| i32.const 0 | ||
| unreachable | ||
| nop | ||
| struct.set $pair 0 |
Member
There was a problem hiding this comment.
Can we not stop parsing entirely at the unreachable? We can skip unreachable code since we don't try to precisely roundtrip anyhow. In fact I believe we used to do that in the past?
Member
Author
There was a problem hiding this comment.
We can't skip parsing instructions after unreachables if we want to be able to write tests containing expressions with unreachable children (which we definitely do). We might have skipped parsing such instructions in the binary parser in the past, but now both text and binary parsers use IRBuilder.
kripken
approved these changes
Apr 20, 2026
Member
Author
|
🤦 this didn't include the commit the remove the stale code from #8616... |
tlively
added a commit
that referenced
this pull request
Apr 20, 2026
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
2 participants
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.