Repository navigation
Conversation
Instead of using a single tuple-typed scratch local for multivalue control flow inputs, allocate one scalar scratch local per input value. This avoids eagerly packaging scalar inputs into a tuple on scope entry. It also avoids allocating extra tuple scratch locals to extract elements when inputs are consumed inside the scope. Scalar input locals do have some downsides. Scopes that pass all of their inputs through untouched must now repackage them with tuple.make. Multivalue loop trampolines in fixLoopWithInput also still need a tuple scratch local to unpack branch values into the scalar input locals. However, real parameterized scopes usually consume their inputs as individual values. Scalar input locals can also be coalesced directly by standard local passes without tuple optimization, and they will allow individual parameters to be tunneled across nested scopes with different arities in a future commit.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the WebAssembly IR builder to support multi-value (tuple) control flow inputs by transitioning from a single scratch local to a vector of scratch locals (inputLocals). This allows proper lowering and extraction of tuple elements. The feedback suggests robustly handling potential allocation failures in fixLoopWithInput by changing its return type to Result<> and propagating errors using CHECK_ERR.
| void fixLoopWithInput(Loop* loop, | ||
| Type inputType, | ||
| const std::vector<Index>& scratch); |
There was a problem hiding this comment.
Since fixLoopWithInput now allocates a new scratch local using addScratchLocal (which returns a Result<Index>), it can fail. To handle this failure robustly and avoid unchecked dereferencing of the Result, change the return type of fixLoopWithInput from void to Result<>.
| void fixLoopWithInput(Loop* loop, | |
| Type inputType, | |
| const std::vector<Index>& scratch); | |
| Result<> fixLoopWithInput(Loop* loop, | |
| Type inputType, | |
| const std::vector<Index>& scratch); |
| // the branch value to the appropriate scratch local. | ||
| fixLoopWithInput(loop, scope.inputType, scope.inputLocal); | ||
| // the branch value to the appropriate scratch locals. | ||
| fixLoopWithInput(loop, scope.inputType, scope.inputLocals); |
There was a problem hiding this comment.
Propagate the error returned by fixLoopWithInput using CHECK_ERR to ensure any allocation failures are handled safely.
CHECK_ERR(fixLoopWithInput(loop, scope.inputType, scope.inputLocals));
| return popConstrainedChildren(children); | ||
| } | ||
|
|
||
| Result<> popInputs(Type inputType, std::vector<Expression*>& inputs) { |
There was a problem hiding this comment.
Is it necessary to mutate the inputs that the caller passed in? Can we return Result<std::vector<Expression*>> instead and remove the param? It looks like the only caller here passes an empty vector anyway.
There was a problem hiding this comment.
Yeah, good idea. The current interface is consistent with popConstrainedChildren, but that's not a public API like this is.
| if (inputType.isSingle()) { | ||
| list.push_back(builder.makeLocalSet(scratch[0], inner)); | ||
| } else { | ||
| auto tupleScratch = *addScratchLocal(inputType); |
There was a problem hiding this comment.
As Gemini mentioned we should check the error here
There was a problem hiding this comment.
I'll add an assertion. It's impossible for addScratchLocal to fail in this case because we cannot have gotten to the point where we are fixing up loop inputs if we are not inside a function context.
Since there are tradeoffs involved here, can we benchmark this against a binary that uses block params? |
Maybe, but only by stacking #9300 and a not-yet-published follow-on PR on top of this and the following PRs, so it would be a little annoying. #9337 (the PR after this one) is the most important preliminary for landing #9300, and it becomes significantly more flexible in conjunction with this change. Regarding the downsides: We won't ever have reason to produce scopes that pass their operands through. We will also likely skip producing loops with input parameters, and even if we did produce them, there would be very few of them in real-world modules (see #9300 (comment)). So the downsides only show up in very rare or contrived cases. |
Instead of using a single tuple-typed scratch local for multivalue control flow inputs, allocate one scalar scratch local per input value. This avoids eagerly packaging scalar inputs into a tuple on scope entry. It also avoids allocating extra tuple scratch locals to extract elements when inputs are consumed inside the scope.
Scalar input locals do have some downsides. Scopes that pass all of their inputs through untouched must now repackage them with tuple.make. Multivalue loop trampolines in fixLoopWithInput also still need a tuple scratch local to unpack branch values into the scalar input locals. However, real parameterized scopes usually consume their inputs as individual values. Scalar input locals can also be coalesced directly by standard local passes without tuple optimization, and they will allow individual parameters to be tunneled across nested scopes with different arities in a future commit.