Support control flow inputs in IRBuilder #7149
Conversation
Since multivalue was standardized, WebAssembly has supported not only multiple results but also an arbitrary number of inputs on control flow structures, but until now Binaryen did not support control flow input. Binaryen IR still has no way to represent control flow input, so lower it away using scratch locals in IRBuilder. Since both the text and binary parsers use IRBuilder, this gives us full support for parsing control flow inputs. The lowering scheme is mostly simple. A local.set writing the control flow inputs to a scratch local is inserted immediately before the control flow structure begins and a local.get retrieving those inputs is inserted inside the control flow structure before the rest of its body. The only complications come from ifs, in which the inputs must be retrieved at the beginning of both arms, and from loops, where branches to the beginning of the loop must be transformed so their values are written to the scratch local along the way. Resolves #6407.
|
|
||
| bool getBasicType(int32_t code, Type& out); | ||
| bool getBasicHeapType(int64_t code, HeapType& out); | ||
| // Get the signature of control flow structure. |
There was a problem hiding this comment.
| // Get the signature of control flow structure. | |
| // Get the signature of a control flow structure. |
|
|
||
| - BinaryenSelect no longer takes a type parameter. | ||
| - AutoDrop APIs have been removed. | ||
| - Binaryen now supports parsing control flow structures with parameter types. |
There was a problem hiding this comment.
| - Binaryen now supports parsing control flow structures with parameter types. | |
| - Binaryen now supports parsing control flow structures with parameter types | |
| (this is not fully optimized, however, and is therefore not recommended). |
There was a problem hiding this comment.
I would expect it to make basically no difference after optimizations, so I'm not sure we need to be this conservative in the changelog note. If producers can get simpler unoptimized modules by using control flow inputs and it basically makes no change to their optimized output, then it's a still a net win.
There was a problem hiding this comment.
True, yeah, so my wording wasn't good. But still, I can imagine someone does a bunch of work to use inputs to control flow, and expect Binaryen to preserve that benefit, which seems surprising when it doesn't. Maybe a note that we do not emit such shapes?
There was a problem hiding this comment.
I expanded the note to say that we lower the input parameters away, which I think is the important information here.
There was a problem hiding this comment.
sgtm though I don't see that pushed?
| } | ||
| WASM_UNREACHABLE("unexpected scope kind"); | ||
| } | ||
| bool isDelimiter() { return getElse() || getCatch() || getCatchAll(); } |
There was a problem hiding this comment.
is Delegate not a delimiter? (if not, what does that term mean here?)
There was a problem hiding this comment.
By delimiter here, I mean a separator between different sections of a control flow structure with multiple bodies. Delegate does not introduce a new control flow body, so it is more like an extra fancy end.
| // Normally an if without an else must have type none, but if there is an | ||
| // input parameter, the empty else arm must propagate its value. | ||
| // Synthesize an else arm that loads the value from the scratch local. | ||
| iff->ifFalse = builder.makeLocalGet(scope.inputLocal, scope.inputType); |
There was a problem hiding this comment.
I don't follow this. There is an input type, but this makes the second arm have an output type. Can this arm simply not read the scratch local?
There was a problem hiding this comment.
Any if without an else must have equivalent input and output types.
In the specified WebAssembly semantics, there's actually no such thing as an if without an else arm. There are only ifs with empty else arms instead. It might be worth looking into removing the ability for ifFalse arms to be null in Binaryen IR; that might be a significant simplification.
There was a problem hiding this comment.
Oh, I see... right, if the input isn't read in an arm, it has to flow out.
There was a problem hiding this comment.
About disallowing null in our IR, interesting idea. It would have some memory usage downsides though. I don't have a good guess if it would help enough to justify that.
Now that #7149 added support for parsing block parameters, we can run additional spec tests that previously failed.
|
(lgtm aside from open discussions) |
The PR was accidentally merged without these fixes included.
Now that #7149 added support for parsing block parameters, we can run additional spec tests that previously failed.
Since multivalue was standardized, WebAssembly has supported not only
multiple results but also an arbitrary number of inputs on control flow
structures, but until now Binaryen did not support control flow input.
Binaryen IR still has no way to represent control flow input, so lower
it away using scratch locals in IRBuilder. Since both the text and
binary parsers use IRBuilder, this gives us full support for parsing
control flow inputs.
The lowering scheme is mostly simple. A local.set writing the control
flow inputs to a scratch local is inserted immediately before the
control flow structure begins and a local.get retrieving those inputs is
inserted inside the control flow structure before the rest of its body.
The only complications come from ifs, in which the inputs must be
retrieved at the beginning of both arms, and from loops, where branches
to the beginning of the loop must be transformed so their values are
written to the scratch local along the way.
Resolves #6407.