Provide Module& parameter to operateOnScopeNameUsesAndSentTypes

[frank-emrich]

This PR performs the necessary plumbing to address a problem plaguing #6083:

In BranchUtils::operateOnScopeNameUsesAndSentTypes, Resume instructions need access to the module to obtain type information associated with tags in order to determine the types of values that may be sent to a block. The same will hold for exception handling instructions, once the updated proposal is implemented.

The most notable user of operateOnScopeNameUsesAndSentTypes is BranchSeeker, whose use sites potentially need to be updated. However, many existing users of BranchSeeker actually just call BranchSeeker::has. The latter can be implemented in terms of operateOnScopeNameUses, rather than operateOnScopeNameUsesAndSentTypes.

Thus, I have split BranchSeeker into two classes:

1. BranchTypeSeeker is like the old BranchSeeker, but without has or count. It is implemented in terms of operateOnScopeNameUsesAndSentTypes and therefore needs to be constructed with a Module&.
2. BranchSeeker provides has and count as static members. It is now implemented in terms of operateOnScopeNameUses, meaning that the two functions can keep their original signatures.

Most changes in this PR then come from the fact that Block::finalize() uses (what is now called) BranchTypeSeeker, and therefore needs to be changed to Block::finalize(Module* wasm).

I have also added Block::finalize()`, which currently does nothing. I have deliberately not tried to identify places where it may be okay to call Block::finalize() and avoid the computation of types that flow into the block.

[kripken]

The root of this sequence of changes appears to be adding Module& wasm here. That leads to needing the module in the new BranchTypeSeeker which leads to Block::finalize needing it, which leads to lots of changes.

However, I do not actually see Module& wasm used here in operateOnScopeNameUsesAndSentTypes, so I don't yet understand the motivation. Reading the code in #6083 I don't see wasm used there. I do see some discussion in https://github.com/WebAssembly/binaryen/pull/6083/files#r1382658351 but I can't understand it 😄 Probably because it speaks about wasm proposals I am not familiar with.

What I am trying to understand is: what would the code look like, that uses Module& wasm here?

I ask because this PR looks like a very large refactoring of the sort we've tried to avoid in general. For example, RefFunc could in theory read the type of the function from the Module, but we purposefully do not do that in ReFinalize::visitRefFunc, and instead rely on it being set initially in a correct manner, and then we just preserve it there (and anyone that changes a Function's type is responsible for updating RefFuncs). In that case it avoids parallelism risks of modifying one function's type while modifying the contents of another, for example - in general, we try to avoid reading global state like this for Expressions.

[tlively]

The core problem is that the new resume instruction from typed continuations and the new try_table instruction from the redesigned EH proposal act as a branches where the sent types depends on tag types. We currently depend on being able to determine the sent type of branches just by inspecting the branching Expression, but the new dependencies on tag types means that determining the types of branches requires being able to look up the tags on the module as well.

[frank-emrich]

The root of this sequence of changes appears to be adding Module& wasm here. That leads to needing the module in the new BranchTypeSeeker which leads to Block::finalize needing it, which leads to lots of changes.

Yes, that's right.

Sorry, I realize that this PR and the linked one do a bad job at showing what the Module& is actually needed for.

From a high-level perspective, the typed continuations proposal allows writing something like this:

(type $ct (cont ...)) 
(tag $mytag (params ...) (results ...))

...
(func ...
  ...
 (block $myblock (results ...)
  (resume $ct (tag $mytag $myblock) (local.get $mycont))
 )
 ...
)

This means that if you suspend to the tag $mytag while running the continuation $mycont, then execution continues after $myblock. The type of the data that is provided to $myblock depends on the types of the tag used and the type of the continuation (the latter is $ct here).

Something very similar will happen for the new EH proposal, where exception handlers are just ordinary blocks, and throw jumps to them, with additional data.

Practically speaking, for operateOnScopeNameUsesAndSentTypes this means that we need to do something like this:

else if (auto* res = expr->dynCast<Resume>) {

...
  Type tag_sig = module.getTags(res->handlerTags[i])->sig;
  <type send to label is calculated from `tag_sig`>
...

}

I agree that this is a rather large refactoring. Id be happy to hear if there are ways of decreasing its footprint somehow.

[kripken]

Thanks for the explanation!

Can we simply duplicate the type into the resume? This is what we do in practice for e.g. RefFunc today: the type of each RefFunc mirrors the module's information about the function's signature. Likewise a Call mirrors the returns of the signature, etc. While in general such duplication is not great, it allows us to avoid reading global state constantly.

If that can't work, maybe elaborate on what i is in that example, and what type send to label is calculated means in practice? (if those things are referring to some kind of "dynamic" read from the module, then simple mirroring of a single value might not work, but I hope that's not the case here...) For the new EH proposal, at least, I think this should work.

[tlively]

Both of the new instructions take a vector of (tag, label) pairs, so they're similar to br_table except that they can potentially send different types to each of the labels. If we want to store the sent types directly in the new Expressions, they would have to be ArenaVector<Type> rather than just a single type.

[frank-emrich]

Can we simply duplicate the type into the resume?

There is nothing prohibiting this from a technical perspective. Note that in general, each resume instruction has a list of (tag $sometag $someblock) clauses, where my example above only used one. This means that we would have to duplicate the Signatures of all of the tags into the Resume object.

If that can't work, maybe elaborate on what i is in that example,

I just used the i to reflect that the Resume node actually contains a list of tags and a list of blocks, together representing a (tag $sometag $someblock) entry. I was trying to hand-wave this away, because operateOnScopeNameUsesAndSentTypes needs some unrelated adapation to avoid a quadratic blowup because of this, but that is independent from the need to have access to the Module.

But coming back to your actual question: The information we need from the Module is entirely static. We just need access to the param and results types that were given when defining the tags in the tags section of the module.
More concretely, the types of the values that flow into a handler block (such as $myblock in my example above) are then a) the param types of the tag used, followed by b) a continuation type. The latter continuation type is determined entirely by the result types of the tag and the type of the resumed continuation (which can be read off from the instruction itself).

[frank-emrich]

If we want to store the sent types directly in the new Expressions, they would have to be ArenaVector rather than just a single type.

Right, in my previous comment I mentioned storing the signtatures of each of the tag in the Resume node for each (tag ...) clause, but we may equally store the sent types directly. Note that these are often tuples, because the types sent to the handler are the param types of the corresponding tag, followed by a continuation type.

[kripken]

I see, thanks, that it's a list of types is the tricky part then.

An ArenaVector<Type> seems like a reasonable way to proceed here, since I don't think we will have very many such nodes and not very long lists in them? For EH at least for C++ I believe the list will always be of length 1 (the single standard C++ exception tag with a pointer in it).

[frank-emrich]

In general, the size of the ArenaVector<Type> itself will be equal to the number of (tag ...) clauses of the resume instruction. Each Type will then be a tuple type with n_i + 1 elements, where n_i is the number of parameters of the tag used by the i-th clause.
I think it's difficult to give a meaningful estimate of how many (tag ...) clauses a resume instruction will usually have, and how many params the involved tags will have. But I would be surprised to see larger numbers of each.

Going back to your original concerns about this refactoring, I don't quite understand what you meant by the following:

In that case it avoids parallelism risks of modifying one function's type while modifying the contents of another, for example - in general, we try to avoid reading global state like this for Expressions.

Were you concerned that we are relying on some kind of global mutable state here? Luckily, this is not the case. Or was your concern more generally about the fact that in, say, Block::finalize(Module& wasm) we would be relying on information that is maintained outside of the block in question? That is definitely true.
During the refactoring, it seemed like the Module was usually in reach and just needed to be passed on to additional places (which arguably then sums up to: quite a lot of additional places).

I don't mind discarding this PR and adding the ArenaVector to the Resume nodes in #6083 if that is your preferred option.

[kripken]

Sorry if I wasn't clear about the parallelism. Yes, in this case it is safe. I was trying to give an example of why we avoid reading global state in general, one reason for which is that in some situations it is unsafe.

I do prefer to add an ArenaVector. It's less of a change, and it is more consistent with our existing coding style, I think. If we see it adds significant overhead in practice we can always optimize it later.

[frank-emrich]

Closing this in favor of storing the required information about types sent to blocks in Resume nodes directly.