[NFC] Refactor ChildLocalizer to handle unreachable code better #6394
Conversation
| // If there is an unreachable child then we do not need the parent at all, | ||
| // and we know the type is unreachable. |
There was a problem hiding this comment.
What if the parent is needed for validation like the post-unreachable children? For example it could be a br_if where the condition is unreachable but where it is the only reason its target block has type none.
There was a problem hiding this comment.
Good point. That is not an issue atm as it is called on things that do not have that behavior, but the user needs to be aware of that. I'll add a note.
There was a problem hiding this comment.
Would it be safer to just include the parent and let later DCE clean it up?
There was a problem hiding this comment.
The caller needs to remove this code in some cases, and doing it there would be parsing the output from here to see if it is a block, and then checking if there is an unreachable child, and then removing the last block element if so (edit: and refinalizing). Then seems more complex to me.
| ;; The fields can be removed here, but the effects must be preserved before | ||
| ;; the struct.new. The consts in the middle can vanish entirely. |
There was a problem hiding this comment.
Why do we try to handle unreachability at all in GTO when a --dce before hand would have the same effect?
There was a problem hiding this comment.
Note that this test does not have unreachability (only conditionally - but nothing unreachable reaches the struct.new).
We do want to handle unreachability in #6395. The situation there is that it is just simpler to allow removing unreachable code too (once we have done the other work in that PR to handle non-unreachable effects).
… with effects (#6395) Before this PR, when we saw a param was unused we sometimes could not remove it. For example, if there was one call like this: (call $target (call $other) ) That nested call has effects, so we can't just remove it from the outer call - we'd need to move it first. That motion was hard to integrate which was why it was left out, but it turns out that is sometimes very important. E.g. in Java it is common to have such calls that send the this parameter as the result of another call; not being able to remove such params meant we kept those nested calls alive, creating empty structs just to have something to send there. To fix this, this builds on top of #6394 which makes it easier to move all children out of a parent, leaving only nested things that can be easily moved around and removed. In more detail, DeadArgumentElimination/SignaturePruning track whether we run into effects that prevent removing a field. If we do, then we queue an operation to move the children out, which we do using a new utility ParamUtils::localizeCallsTo. The pass then does another iteration after that operation. Alternatively we could try to move things around immediately, but that is quite hard: those passes already track a lot of state. It is simpler to do the fixup in an entirely separate utility. That does come at the cost of the utility doing another pass on the module and the pass itself running another iteration, but this situation is not the most common.
2 participants
This is NFC in the current users, but is necessary functionality for a later
PR.
ChildLocalizer moves children into locals as needed. It used to stop when it
saw the first unreachable. After this change we move such unreachable
children out of the parent as well, making this more uniform: all interacting
effects are moved out, and all that is left nested in the parent can be
moved around and removed as desired.
Also add a
getReplacementhelper that makes using this easier.This cannot be tested comprehensively with the current user as that user
will not call this code path on an unreachable parent at all, so this just
adds what can be tested. The later PR will have tests for all corner cases
🤞