Avoid new blocks in binary reading/writing

[kripken]

Background: wasm allows lists at the top of functions, in loop bodies, and in if arms. Binaryen IR only allows a list in a block (so passes only need to deal with lists in one place), and as a result we may need more blocks than wasm does in some cases.

Turns out we generated more than we needed, which is kind of silly since just reading and writing a binary repeatedly could lead to an unstoppable increase in size. This PR fixes that. One commit is for writing, one is for reading.

[kripken]

amusingly this was wrong all along, so the optimization never kicked in (if it did, it would have led to breakage, which is fixed in this PR - we need to call getBlock in places where a branch might occur, like the top of a function)

[kripken]

This has been fuzzed heavily and looks correct. Any concerns?

[dschuff]

This name is confusing. There is no Expression that's a list, other than a block. GetMaybeBlock makes sense if only because it could return either a block or a single instruction.

[dschuff]

I guess what this means is that it could return a single instruction or an implicit block (which cannot be branched to, and will not appear in the wasm output)?

[kripken]

Yeah, exactly - it's reading a "list context" in a wasm binary, so it could be a list of instructions, but we do know it can't be branched to.

Ideas for a better name? :) Maybe getExpressionList or getExpressionOrList?

[dschuff]

I don't really understand what you mean by "potential" here. This always returns an actual block, no? Is the difference just that it may or may not be an implicit block (e.g. a function body or if arm?

[dschuff]

Ok, I see that's wrong. So the only difference from getList is that this could return a single instruction or an explicit block (that can be branched to).

[kripken]

Yeah. Both can return a block or a single instruction if a block isn't needed. A block might be needed if we branch to it (in getBlock, but not getList where we assume no branches), or if we have more than one instruction.

[kripken]

How about getUnnamedBlockOrSingleton() for getting either a singleton or a block, where the block can't be branched to so we create it without a name, and getBlockOrSingleton() for the case where we can branch to it?

[dschuff]

OK, I think I understand better. In both cases you want an expression or list of expressions. In one case (function top level, explicit block, if arm), you need it to be targetable by a branch. In the other case, it must not be targetable because its container is the one that's targetable (loop). In both cases you want to just use a bare expression if there are in fact no branches that target it.

Given that, I have 2 questions:

1. Why does the logic have to be duplicated rather than shared (and have a parameter or something)
2. Why are if-arms in the first category instead of the second? i.e. instead of containing blocks with names, why aren't the arms targetable the way loops are? Maybe because the jump is forward instead of back?

[kripken]

Which logic do you mean is duplicated? if you mean the identical lines in getList and getBlock, it's just a few, and it's simpler that way I think. Otherwise getList could get a parameter, but it would be kind of messy to check it multiple times.

About loops, yeah, the issue is that their branch is backwards. wasm no longer lets them be broken out of - they only have a top label, in other words. getBlock optionally creates a block, which is only a forwards branch. So in loops we getList, and handle the branch to the loop top directly.

[dschuff]

ok. the code seems ok; really getList is really only for loops (everything else can be branched to) so maybe just say in the comment directly that it's for loops because its branch target is on the loop instead of the block.

Or actually, even better: let's just put that logic directly in visitLoop, and then maybe getNextExpressionOrBlock or getBlockOrSingleton or something like that for getBlock

[kripken]

I like that, nice. Updated to getBlockOrSingleton(), and the loop-specific getList logic is now inside the loop code, and that name is gone.