[Outlining] Fixes break reconstruction #6352
Conversation
| Type type = expr->value ? expr->value->type : Type::none; | ||
| ASSERT_OK(builder->visitSwitchWithType(expr, type)); | ||
| } else { | ||
| ASSERT_OK(builder->visit(curr)); |
There was a problem hiding this comment.
It would be good to add an assertion somewhere to future-proof ourselves against the possibility that a new kind of Expression starts to need a visit*WithType variant. This could either be an assert(!Properties::isBranch(curr)) here guarding the default builder->visit(curr) case, or it could be an assertion that curr is a Break or Switch inside IRBuilder::getBranchValue.
src/wasm-ir-builder.h
Outdated
| // with a corresponding push onto the scope stack. As a result, an exception | ||
| // will occur if a corresponding scope is not found for the break. |
There was a problem hiding this comment.
Probably not an exception, right? Will we return an error in this case?
There was a problem hiding this comment.
You're right, not an exception. Updated!
src/wasm-ir-builder.h
Outdated
| [[nodiscard]] Result<> visitBreak(Break*, | ||
| std::optional<Index> label = std::nullopt); | ||
| // Used to visit break nodes when traversing the module in the stacky format. |
There was a problem hiding this comment.
I think this would be more precise.
| // Used to visit break nodes when traversing the module in the stacky format. | |
| // Used to visit break nodes when traversing a single block without its context. |
src/wasm-ir-builder.h
Outdated
| // The type indicates how many values need to be popped from the stack and | ||
| // consumed by the break. |
There was a problem hiding this comment.
It's important that the condition (if any), which is also popped from the stack, is not included in this.
| // The type indicates how many values need to be popped from the stack and | |
| // consumed by the break. | |
| // The type indicates how many values the break carries to its destination. |
Similar comments apply to visitSwitchWithType below.
src/wasm/wasm-ir-builder.cpp
Outdated
| CHECK_ERR(cond); | ||
| curr->condition = *cond; | ||
| } | ||
| if (!curr->value) { |
There was a problem hiding this comment.
The type should be the only source of truth here.
| if (!curr->value) { | |
| if (type == Type::None) { |
src/wasm/wasm-ir-builder.cpp
Outdated
| // TODO: Call more efficient versions of finalize() that take the known type | ||
| // for other kinds of nodes as well, as done above. | ||
| ReFinalizeNode{}.visit(curr); |
There was a problem hiding this comment.
Since we know what kind of expression this is, we can just call finalize directly. There's also no more efficient variant for Break.
| // TODO: Call more efficient versions of finalize() that take the known type | |
| // for other kinds of nodes as well, as done above. | |
| ReFinalizeNode{}.visit(curr); | |
| curr->finalize(); |
Same for Switch below.
| ;; CHECK-NEXT: (call $outline$) | ||
| ;; CHECK-NEXT: ) | ||
| ;; CHECK-NEXT: ) | ||
| (func $a |
There was a problem hiding this comment.
It would be good to add a test for the br_table case as well.
Adds new visitBreakWithType and visitSwitchWithType functions to the IRBuilder API. These functions work around an assumption in IRBuilder that the module is being traversed in the fully nested format, i.e., that the destination scope of a break or switch has been visited before visiting the break or switch. Instead, the type of the destination scope is passed to IRBuilder.