LinearExecutionTraversal: Add an option to connect adjacent code, use in SimplifyLocals #5860
Conversation
| ;; YES: (type $A$2 (sub $A (struct ))) | ||
|
|
||
| ;; YES: (type $A$1 (sub $A (struct ))) | ||
|
|
There was a problem hiding this comment.
This is unrelated, and is a change I've noticed for a while now but didn't have time to look into. I think perhaps we just updated this file manually and not with the auto updater. This is what the auto updater adds for it.
| @@ -44,14 +44,49 @@ struct LinearExecutionWalker : public PostWalker<SubType, VisitorType> { | |||
| self->noteNonLinear(*currp); | |||
| } | |||
|
|
|||
| // Optionally, we can connect adjacent basic blocks. "Adjacent" here means | |||
There was a problem hiding this comment.
This can be basically extended to any dominator-dominatee relationship, but given that this is a linear execution walker, we limit the scope to the adjacent blocks, correct?
I thought about whether "consecutive" sounds better, but usually this word doesn't contain the meaning that the first can branch out or return, so I'm not sure.
There was a problem hiding this comment.
Yes, exactly, it's the simple case during linear execution when we see dominator-dominatee.
| // It is ok to look at adjacent blocks together, as if a later part of a | ||
| // block is not reached that is fine - changes we make there would not be | ||
| // reached in that case. | ||
| bool connectAdjacentBlocks = true; |
There was a problem hiding this comment.
Are the cases where we need this the same as where we need bool ignoreBranchesOutsideOfFunc = true; for cfg-traversal.h?
There was a problem hiding this comment.
I think it's a little different. I was actually thinking of saying in a comment "this is like ignoreBranchesOutsideOfFunc" but it seemed like it could be more confusing than helpful, though I'm not sure...
The main difference is there we ignore branches outside of the function. We never ignore a branch inside it. So we use that in cases where we just care about state in locals, for example, that goes away when we branch out of the function. We can use that for local coalescing for that reason. But we couldn't use this PR for local coalescing since we need to be aware of every branch inside the function, or else we could miss some interferences.
There was a problem hiding this comment.
Isn't this PR also about excluding branches out of a function? Ah, I guess it is branch within a function in case of if..?
There was a problem hiding this comment.
Yes, exactly, an if would be inside. I think I can also handle br_if etc. but I'll leave that for later.
Codecov Report
@@ Coverage Diff @@
## main #5860 +/- ##
=======================================
Coverage ? 42.41%
=======================================
Files ? 482
Lines ? 74954
Branches ? 11941
=======================================
Hits ? 31793
Misses ? 39949
Partials ? 3212 |
) Followup to #5860, this does the same for SimplifyGlobals as for SimplifyLocals. As there, this is valid because it's ok if we branch away. This part of the pass applies a global value to a global.get based on a dominating global.set, so any dominance is good enough for us.
Followup to #5860, this does the same for LocalCSE. As there, this is valid because it's ok if we branch away. This pass adds a local.tee of a reused value and then gets it later, and it's ok to add a tee even if we branch away and do not use it.
Followup to #5860, this does the same for (part of) OptimizeCasts. As there, this is valid because it's ok if we branch away. This part of the pass picks a different local to get when it knows locals have the same values but one is more refined. It is ok to add a tee earlier even if it isn't used later.
Br and BrOn can consider the code before and after them connected if it might
be reached (which is the case if the Br has a condition, which BrOn always has).
The wasm2js changes may look a little odd as some of them have this:
i64toi32_i32$1 = i64toi32_i32$2;
i64toi32_i32$1 = i64toi32_i32$2;
I looked into that and the reason is that those outputs are not optimized, and
also even in unoptimized wasm2js we do run simplify-locals once (to try to
reduce the downsides of flatten). As a result, this PR makes a difference there,
and that difference can lead to such odd duplicated code after other operations.
However, there are no changes to optimized wasm2js outputs, so there is no
actual problem.
Followup to #5860.
Enough of an improvement happened to cause some expectations to fail. Largest improvements are due to recent Binaryen changes (WebAssembly/binaryen#5860 and followups).
This addresses most of the minor regression from the correctness fix in #5857.
That PR makes us consider calls as branching, but in some cases it is ok to
ignore that branching (see the comment in the code), which this PR allows as
an option.
This undoes one test change from that PR (showing it undoes the regression).
More tests are added to cover this specifically as well.