Avoid adding new unneeded names to blocks in text roundtripping #4943
Conversation
| if (!hadName && !BranchUtils::BranchSeeker::has(curr, curr->name)) { | ||
| curr->name = Name(); | ||
| } |
There was a problem hiding this comment.
I think it would be fairly simple to avoid the quadratic behavior of ``BranchSeeker` by keeping a set of referenced names as we build branch expressions.
There was a problem hiding this comment.
Yeah, that could be done. It would add complexity though, and this is not a hot code path - we only seek here if there never was a name. Blocks without a name tend to be rare workarounds for stacky code etc., and they do not occur in the actually dangerous situation of a tall tower of blocks for a switch - all the blocks there are named. Also, this is in the s-parser, which is not heavily optimized anyhow.
| (drop | ||
| (i32.const 0) | ||
| ) | ||
| (nop) |
There was a problem hiding this comment.
Do you know where these extra (nop)s are coming from?
There was a problem hiding this comment.
Good point, those shouldn't be there. This looks like a minor issue uncovered by this PR. It's not a regression on real-world code since this PR just affects text inputs, so I'll land this, but I'll fix it as a followup.
This fixes what looks like it might be a regression in #4943. It's not actually an issue since it just affects wat files, but it did uncover an existing inefficiency. The situation is this: (block .. (br $somewhere) (nop) ) Removing such a nop is always helpful, as the pass might see that that br goes to where control flow is going anyhow, and the nop would confuse it. We used to remove such nops only when the block had a name, which is why wat testcases looks optimal, but we were actually doing the less-efficient thing on real-world code. It was a minor inefficiency, though, as the nop is quickly removed by later passes anyhow. Still, the fix is trivial (to always remove such nops, regardless of a name on the block or not).
Previously the wat parser would turn this input:
into something like this:
It just added a name all the time, in case the block is referred to by an index
later even though it doesn't have a name.
This PR makes us rountrip more precisely by not adding such names: if there
was no name before, and there is no break by index, then do not add a name.
In addition, this will be useful for non-nullable locals since whether a block has
a name or not matters there. Like #4912, this makes us more regular in our
usage of block names.
Unfortunately the diff here is huge. But 99% of it is in the very first commit,
which aside from the small code change is 100% automatic. The small commits later
are where I had to add some manual changes (as some tests actually depended
on the old behavior, and I wanted to keep the tests doing the same as much as
possible). Reading them might be more useful (but they are relative to the big
initial comment, and undo changes there, so looking at the same file in the total
diff might be interesting).