Repository navigation
Conversation
Skip `Preoptimizer::visitRefCast`, `visitRefGetDesc`, and `visitBrOn` when an operand is unreachable (either already unreachable in the input or replaced with an unreachable block earlier in `Preoptimizer`), updating `curr->type` to `Type::unreachable` so parent expressions also see the unreachability and `ReFinalize` cleans up unreachable branches. This fixes a bug where Preoptimizer was replacing unreachable expressions with none-typed expressions, leading to validation failures.
There was a problem hiding this comment.
Code Review
This pull request updates the AbstractTypeRefining pass to correctly handle cases where operands of RefCast, RefGetDesc, or BrOnCast instructions are optimized to unreachable. In these scenarios, the instruction's type is now explicitly set to unreachable to prevent validation or optimization issues. Corresponding test cases have been added to verify these changes. I have no feedback to provide as there are no review comments.
| if (curr->op == BrOnNull || curr->op == BrOnNonNull) { | ||
| return; | ||
| } | ||
| if (curr->ref->type == Type::unreachable || |
There was a problem hiding this comment.
[HIGH | Confidence: 5/5 | BUG]
The unreachable check in visitBrOn is placed after the early return for BrOnNull and BrOnNonNull:
| if (curr->ref->type == Type::unreachable || | |
| if (curr->op == BrOnNull || curr->op == BrOnNonNull) { | |
| return; | |
| } | |
| if (curr->ref->type == Type::unreachable || | |
| (curr->desc && curr->desc->type == Type::unreachable)) { | |
| curr->type = Type::unreachable; | |
| return; | |
| } |
If the ref operand of a BrOnNull or BrOnNonNull became unreachable during the post-order traversal (e.g., from an uninstantiated RefGetDesc replaced with an unreachable sequence), line 442 returns early without updating curr->type to Type::unreachable.
If this BrOn expression is subsequently visited as the ref child of an enclosing RefCast, RefGetDesc, or another BrOn, the enclosing expression will observe curr->ref->type as a reachable concrete type instead of unreachable. This bypasses the enclosing expression's unreachable guard and causes it to perform invalid transformations (e.g., localizeChildren extracting the branch to a local, or makeRefCast), leading to invalid IR or assertion failures during finalization.
To ensure consistent type propagation matching BrOn::finalize(), move the unreachable check before the operation check:
| if (curr->ref->type == Type::unreachable || | |
| void visitBrOn(BrOn* curr) { | |
| if (curr->ref->type == Type::unreachable || | |
| (curr->desc && curr->desc->type == Type::unreachable)) { | |
| curr->type = Type::unreachable; | |
| return; | |
| } | |
| if (curr->op == BrOnNull || curr->op == BrOnNonNull) { | |
| return; | |
| } | |
| auto optimized = getOptimized(curr->castType); |
There was a problem hiding this comment.
Didn't quite get the comment here, but it at least makes sense to me to move the check up like it suggests? Although maybe refinalize would do this anyway
| void visitRefCast(RefCast* curr) { | ||
| if (curr->ref->type == Type::unreachable || | ||
| (curr->desc && curr->desc->type == Type::unreachable)) { | ||
| curr->type = Type::unreachable; |
There was a problem hiding this comment.
Why is this needed? Did we modify the code at this point but not call refinalize yet? If so it seems worth a comment as in general this should not be necessary.
Skip
Preoptimizer::visitRefCast,visitRefGetDesc, andvisitBrOnwhen an operand is unreachable (either already unreachable in the input or replaced with an unreachable block earlier inPreoptimizer), updatingcurr->typetoType::unreachableso parent expressions also see the unreachability andReFinalizecleans up unreachable branches.This fixes a bug where Preoptimizer was replacing unreachable expressions with none-typed expressions, leading to validation failures.