-
Notifications
You must be signed in to change notification settings - Fork 830
CoalesceLocals: Rewrite the algorithm to be linear and to ignore copies #4314
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| ;; CHECK-NEXT: (local.tee $0 | ||
| ;; CHECK-NEXT: (local.tee $1 | ||
| ;; CHECK-NEXT: (local.tee $0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this:
(local.tee $0
(local.tee $0
...
)
)Also could be optimized as duplicate (redundant) operation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MaxGraey Yes, see the TODO here: https://github.com/WebAssembly/binaryen/pull/4314/files#diff-4b82a7f223b291ea9808e718d45e91593dc75d9bf684d42386e0f0f480132a16R518 I didn't want to add this in this PR to keep it smaller, but that's a followup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, now I see
tlively
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! The code here is easier to follow than I would have expected.
| // This is a set. Find the value being assigned to the local. | ||
| auto* set = (*action.origin)->cast<LocalSet>(); | ||
| Index newValue; | ||
| if (set->value->is<LocalGet>() || set->value->is<LocalSet>()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we get the fallthrough value here? Separately, could we consider the case where the value is the default value for the relevant type and set the value back to zeroInit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good ideas, adding as TODOs for myself.
Co-authored-by: Thomas Lively <7121787+tlively@users.noreply.github.com>
These gains come from WebAssembly/binaryen#4314
These gains come from WebAssembly/binaryen#4314
|
Any chance that this will address also: #3646 ? |
|
@trzecieu Hmm, I'm afraid not, this helps with nonlinear runtimes but that issue is about nonlinear memory usage in the case with huge amounts of locals, which would require a different fix (I see there are some ideas in that issue, but I'm not aware of anyone working on them atm). |
This removes the old hardcoded value numbering in that pass and makes it use the new code that was split into helper code. The immediate benefit of this is to make the code aware of identical constants: if two locals have the same constant then they do not interfere. Future improvements to numbering will also automatically help here. This changes some constants in existing tests so that they keep testing what they were testing before, and adds new tests for the new benefit here. This implements a proposed TODO from #4314
The old algorithm can be summarized as: In each basic block, start at the beginning.
Each pair of live locals there might interfere with each other, as they might arrive from
different entry blocks with different values. Afterwards, go through the block and find
overlapping live ranges, and mark interferences there as well.
This is non-linear because at the start of the block we do a double-loop over all
pairs of live locals, which in general can be
O(N^2)(N - number of locals). It alsohas the downside of ignoring copies: if two locals have overlapping live ranges but
they must have identical values on those ranges, they do not actually interfere,
for example
We can ignore this overlap since the copy shows they are identical there, but the
pass did not take this into account. To some extent other passes can remove such
copies (SimplifyLocals, MergeLocals, RedundantSetElimination), but in general
this was a weak spot for the optimizer.
I realized there is a solution to both these problems: In Wasm, given that we have
a default value for all locals, if a local is live at the start of a block then it must be
live at the end of all the blocks reaching it. That is so because the liveness will
extend backwards all the way to some set of the local, possibly all the way to
the zero-initialization at the start of the function, and it extends that way through
all predecessor blocks. A consequence of this is that there are no interferences
between locals that only occur during a merge: The live ranges include the
predecessor blocks, and theirs, and so forth, until we reach a block where one
of the locals is assigned a value different than the other. That is a necessary and
sufficient condition for intererence, and therefore when processing a block we
only need to look at its contents, and can ignore the merging of control flow,
which allows us to be linear.
More details on this and on the new algorithm in comments in the source, but
the basic idea is that it simply goes through each block in a linear way, finding
which values are assigned to each local (using a numbering of unique values),
and noting which are live at each time. If two locals are live and one is assigned
a value that is not the same as the value in the other, mark them as interfering.
This is of substantial benefit to j2wasm output, I believe because it is common
there to find local subexpression elimination opportunities after inlining, and
each time we find one we add a local. If we inline different functions into the
same target, we may end up with copied locals for each of them. (This was
not noticed in the past because it is very rare on LLVM output, which has
already had inlining and GVN etc. done.) I measured the following benefits:
local.gets.local.sets.struct.sets.The last is because these extra copies were standing in the way of
optimizations like #4244 which require a very particular pattern in
order to safely fold a
struct.setinto astruct.new.There is a small benefit to LLVM output as well, though just a few
percent at best. However, it is enough to be noticeable on some of
the code size tests (which we will need to update after this lands,
fyi @sbc100 ).
This is also faster than the previous pass. It's normally not noticeable
as this pass is not one of the slowest anyhow, but I found some real-world
codebases where the pass becomes 50% faster. I have not found any
case where it is slower than the old algorithm.
Fuzzed over several days to be sure this is correct, and also verified
on the emscripten test suite.
(Note for code review: This causes some large updates to one of the
large existing tests. It's unfortunate, but any change to local indexes
can lead to that, so that test is not that useful here. Might be worth
skimming, but I wouldn't ask that you review that one in depth.)