MergeBlocks: Optimize all dropped blocks #6984
Conversation
Co-authored-by: Heejin Ahn <aheejin@gmail.com>
| @@ -434,6 +434,15 @@ struct MergeBlocks | |||
| optimizeBlock(curr, getModule(), getPassOptions(), branchInfo); | |||
| } | |||
|
|
|||
| void visitDrop(Drop* curr) { | |||
There was a problem hiding this comment.
What's the difference between the cases handled by this and this code in optimizeBlock?
binaryen/src/passes/MergeBlocks.cpp
Line 262 in 7f30b6c
Can't all cases be handled by this new visitDrop?
There was a problem hiding this comment.
The code in optimizeBlock runs on the children of a block, and we do need to run that more than once, unlike visitDrop: Each time we make a signficant change to a block, it's possible that merging with children of the block is possible, so we call optimizeBlock on such occasions.
On the other hand, for things whose parent isn't a block, the new visitDrop is enough, since the parent can't be merged with it, so we can just do that once.
There was a problem hiding this comment.
Actually, I was completely wrong here! You were right, we can remove this code. It looks like changes in the parent never lead to new improvements for the child, so we can just do the visitDrop part once.
I'll do some fuzzing to verify this.
There was a problem hiding this comment.
Ah, actually that was wrong, it turns out. There is at least one case where we do end up improving a child based on the parents, which I added a testcase for now (after reverting the last commit). What seems to happen is that after we optimize the parent there, we then look at the child and see that it can be removed with its name. (This was not noticed in the test suite since we run --remove-unused-names.)
Logically I still think it makes sense that we only need the call from visitDrop, but that would require changes to the pass, and I'm not sure it's worth a large refactoring.
This reverts commit ba7f20d.
When I refactored the optimizeDroppedBlock logic in #6982, I didn't move the unreachability check with that code, which was wrong. When that function was called from another place in #6984, the fuzzer found an issue. Diff without whitespace is smaller. This reverts almost all the test updates from #6984 - those changes were on blocks with unreachable children. The change was safe on them, but in general removing a block value in the presence of unreachable code is tricky, so it's best to avoid it. The testcase is a little bizarre, but it's the one the fuzzer found and I can't find a way to generate a better one (other than to reduce it, which I did).
… too (#6994) In #6984 we optimized dropped blocks even if they had unreachable code. In #6988 that part was reverted, and blocks with unreachable code were ignored once more. However, I realized that the check was not actually for unreachable code, but for having an unreachable child, so it would miss things like this: (block (block .. (br $somewhere) ;; unreachable type, but no unreachable code ) ) But it is useful to merge such blocks: we don't need the inner block here. To fix this, just run ReFinalize if we change anything, which will propagate unreachability as needed. I think MergeBlocks was written before we had that utility, so it didn't use it... This is not only useful for itself but will unblock an EH optimization in a later PR, that has code in this form. It also simplifies the code by removing the hasUnreachableChild checks.
Just call
optimizeDroppedBlockfromvisitDropto handle that.Followup to #6982. This optimizes the new testcase added there. Some older
tests also improve, view without whitespace for a much smaller diff, as
the changes are just to remove some unneeded blocks.