Lower branches with unsupported extra values in IRBuilder #7202
Conversation
WebAssembly allows all branch instructions to carry an arbitrary number of values, but Binaryen IR only supports this for br, br_on, and br_table. Previously we failed to parse any other branches that sent additional arguments. To be able to parse more standard modules, support extra values sent by other branches by lowering them away using scratch locals and trampolines in IRBuilder. For now, only lower BrOn expressions with extra values. Lowering for other branches will be done in follow-on commits. WIP because there are still several tests to write. The existing tests suffice to get a sense of how horrible these lowerings are, though.
tlively
changed the title
|
@kripken This should be ready for real review now. |
| } | ||
| static ScopeCtx | ||
| makeTryTable(TryTable* trytable, Name originalLabel, Type inputType) { | ||
| return ScopeCtx(TryTableScope{trytable, originalLabel}, inputType); | ||
| } | ||
|
|
||
| void resetForDelimiter(bool keepInput) { |
There was a problem hiding this comment.
Perhaps add a comment for this? Reading e.g. makeElse it's not obvious to me what this does, or what "input" refers to in "keepInput".
src/wasm/wasm-ir-builder.cpp
Outdated
| if (auto* block = curr->dynCast<Block>(); | ||
| block && (!block->name || block->name == label)) { |
There was a problem hiding this comment.
I should undo this change.
| @@ -1058,14 +1039,14 @@ Result<> IRBuilder::visitEnd() { | |||
| this->func = nullptr; | |||
| blockHint = 0; | |||
| labelHint = 0; | |||
| } else if (auto* block = scope.getBlock()) { | |||
| assert(*expr == block); | |||
There was a problem hiding this comment.
I should put the assertion back.
| break; | ||
| } | ||
|
|
||
| // If the value under test is unreachable, then we can proceed without putting |
There was a problem hiding this comment.
Is it worth doing this? We could leave it for dce.
There was a problem hiding this comment.
We have to bail out here because we don't know what types to use for the scratch locals. I'll clarify the comment.
| (import "env" "i32" (global $i32 (mut i32))) | ||
| ;; CHECK: (import "env" "i32" (global $i64 (mut i64))) | ||
| ;; OPT_O: (import "env" "i32" (global $i64 (mut i64))) | ||
| (import "env" "i32" (global $i64 (mut i64))) |
There was a problem hiding this comment.
| (import "env" "i32" (global $i64 (mut i64))) | |
| (import "env" "i64" (global $i64 (mut i64))) |
purely for aesthetic reasons
|
About testing, we won't be getting any fuzzing support for this, unfortunately, since it isn't in the IR. Perhaps we could add a fuzz generator that emits stacky wat or something like that, but it sounds like a lot of work. Another option might be to bundle a few wasm files with such code, that would then at least be used as initial content (but they'd be lowered immediately, so not mutated on the form that matters most). |
|
Another option would be to use |
|
Makes sense, spec tests seem best. lgtm with the comments above (spec tests can be later, either way). |
|
I'll look into spec tests as a follow-up. |
WebAssembly allows all branch instructions to carry an arbitrary number
of values, but Binaryen IR only supports this for br, br_if, and
br_table. Previously we failed to parse any other branches that sent
additional arguments. To be able to parse more standard modules, support
extra values sent by other branches by lowering them away using scratch
locals and trampolines in IRBuilder. For now, only lower BrOn
expressions with extra values. Lowering for other branches will be done
in follow-on commits.
Fixes #7182.