Add EH support for CodeFolding #2665
Conversation
This does two things: - Treats the target branch of `br_on_exn` as unoptimizables, because it is a conditional branch. - Makes sure we don't move expressions that contain `exnref.pop`, which should follow right after `catch`. For this, this adds `containsChild` utility function, which can search all children, but for this purpose we do a limited BFS of depth 2 (So far all code generated by toolchain has `exnref.pop` at depth 1. We do 2 here for extra safety measure.
src/passes/CodeFolding.cpp
Outdated
| // pseudo instruction following a catch. In practice, the exnref.pop is a | ||
| // child of a local.set or a rethrow. We check if the current expression | ||
| // has a pop child within a limited depth (2). | ||
| if (containsChild<Pop>(item, 2)) { |
There was a problem hiding this comment.
IIUC, this is meant to catch things like this, at a depth of 1?
(local.set $x (i32.pop))
What happens with say
(local.set $x
(try ..
(catch
(local.set $x (i32.pop))
In this case there is a more deeply nested pop, but it's ok to ignore, I think?
What I'm unsure of is if the 2 here is an optimization, or it's chosen for correctness as we think there is no nesting of >2 that is unsafe, or something like that?
There was a problem hiding this comment.
What happens with say
(local.set $x (try .. (catch (local.set $x (i32.pop))In this case there is a more deeply nested
pop, but it's ok to ignore, I think?
You mean, moving the whole outer local.set together? It should be safe right? Unless we try to cut the bond between catch and local.set, I think it's fine. Is my understanding of your question correct?
What I'm unsure of is if the 2 here is an optimization, or it's chosen for correctness as we think there is no nesting of >2 that is unsafe, or something like that?
It is an optimization. Currently the only toolchain that generates exception code is our LLVM toolchain and it only generates in the form of (local.set $x (exnref.pop)). So I think 1 is fine, but 2 is for an extra measure. I don't think we can afford to search all possible children to unlimited depth for every single expression.
The form of code it generates may change after we support values entering blocks, which I'm not sure how will be represented in Binaryen AST. In that case, if we get to generate other forms of code, we may have to change the limit, but for now I think it's fine.
There was a problem hiding this comment.
So yes, I think moving the entire set in my example is safe. But also that set has a pop at a depth >2 - so it seems to indicate that this isn't just an optimization, as if we ignored the optimization and looked at all depths, we'd not optimize this case.
What concerns me about 2 is that doing a depth of 1, and also when !NDEBUG to check for all depths for an assertion, would not work - the assertion would fire. But that is a natural type of assertion to add here, if 2 is not an optimization.
I think we can do probably do something precise and complicated here, but probably that's not worth it. Maybe it's not that bad to check all depths - it's just a linear traversal of each child, and theoretically quadratic with nested blocks, but very low likelihood to have a significant amount of that?
There was a problem hiding this comment.
We talked about this in person, and decided that limiting depth to 2 is not guaranteed to be safe (Even though the current LLVM wasm toolchain does not generate code that has pop in depth more than 1, we can't guarantee that in general, and there can be other toolchains that support wasm EH later). We decided to remove the depth limit, which is overly conservative but at least is correct.
|
|
||
| // Returns true if the current expression contains a certain kind of expression, | ||
| // within the given depth of BFS. If depth is -1, this searches all children. | ||
| template<typename T> bool containsChild(Expression* parent, int depth = -1) { |
There was a problem hiding this comment.
When the depth is not limited, I think it would be more efficient to not use the ChildIterator, and do a normal traversal. FindAll gets a list of all the nodes, so maybe using that, or something similar to that but without saving the list?
(But this looks useful for the case where the depth is limited, which we'll probably need eventually, so I'm not suggesting we remove it.)
This can perhaps be moved to ir/find_all.h as it seems closely related?
There was a problem hiding this comment.
Didn't know we have FindAll! Done.
2 participants
This does two things:
br_on_exnas unoptimizables, because itis a conditional branch.
exnref.pop, whichshould follow right after
catch.containsChildutility function, which can search all children,optionally with limited depth. This was actually added to be used in
CodeFolding but ended up not being used, but wasn't removed in case
there will be uses later.