Cranelift: fix multiplicity analysis of multi-def instructions. - #14324
Open
cfallin wants to merge 1 commit into
Open
Cranelift: fix multiplicity analysis of multi-def instructions.#14324cfallin wants to merge 1 commit into
cfallin wants to merge 1 commit into
Conversation
This is a follow-up that actually finishes the work of bytecodealliance#14272 and fixes bytecodealliance#14317 as a result. It turns out that I had not connected the dots that *instructions* are the unit onto which multiplicity should attach, not values, because multiplicity is all about how many times a given computation can be lowered. (And then preventing sinking operations that can only happen once, like loads, in contexts with multiplicity.) Use of any output of an instruction can cause that instruction to be lowered; counting multiplicity of `Once` on each of its outputs (and deciding lowering accordingly) is not correct the instruction could be lowered once for *each* of those separate uses and then *its* args might merge in and be lowered more than once. This does regress the load-sinking-into-i128-ops that bytecodealliance#9510 originally aimed to enable. (I had suspected it should have regressed earlier but failed to dig all the way on that hunch; the above inst-vs-value issue hid it.) I am positing that that regression is something we should accept: a clear mental model is more important. And the problem is fundamentally an NP-hard tiling problem (to see why, observe that there is no optimal substructure: earlier lowering decisions change whether later lowerings *actually* happen more than once, so earlier suboptimal lowerings could enable later better lowerings/sinkings or vice versa). The "roots" thing was a bandaid that was choosing one particular greedy approach to the NP-hard problem. This PR and bytecodealliance#14272 together go back to the more direct greedy approach that is easier to articulate: (i) we compute how many times an instruction *could* be lowered, in the worst case, if isel on any uses matches multiple levels deep; (ii) we don't ever permit isel to combine potentially-multiple-lowered operands when they have side-effects. (If we decide otherwise, and want to keep i128+load sinking, I believe we have to delete all of our uadd/umul-overflow lowerings that combine flags, because those fundamentally can lower a multi-def inst more than once, which goes against the whole premise of bytecodealliance#9510. We just have to always materialize flags in that case. And per earlier discussions, peephole can't solve it (because elab can put other insts "in the middle") so we have no other solution to that problem.) Fixes bytecodealliance#14317.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a follow-up that actually finishes the work of #14272 and fixes #14317 as a result.
It turns out that I had not connected the dots that instructions are the unit onto which multiplicity should attach, not values, because multiplicity is all about how many times a given computation can be lowered. (And then preventing sinking operations that can only happen once, like loads, in contexts with multiplicity.) Use of any output of an instruction can cause that instruction to be lowered; counting multiplicity of
Onceon each of its outputs (and deciding lowering accordingly) is not correct the instruction could be lowered once for each of those separate uses and then its args might merge in and be lowered more than once.This does regress the load-sinking-into-i128-ops that #9510 originally aimed to enable. (I had suspected it should have regressed earlier but failed to dig all the way on that hunch; the above inst-vs-value issue hid it.)
I am positing that that regression is something we should accept: a clear mental model is more important. And the problem is fundamentally an NP-hard tiling problem (to see why, observe that there is no optimal substructure: earlier lowering decisions change whether later lowerings actually happen more than once, so earlier suboptimal lowerings could enable later better lowerings/sinkings or vice versa). The "roots" thing was a bandaid that was choosing one particular greedy approach to the NP-hard problem. This PR and #14272 together go back to the more direct greedy approach that is easier to articulate: (i) we compute how many times an instruction could be lowered, in the worst case, if isel on any uses matches multiple levels deep; (ii) we don't ever permit isel to combine potentially-multiple-lowered operands when they have side-effects.
(If we decide otherwise, and want to keep i128+load sinking, I believe we have to delete all of our uadd/umul-overflow lowerings that combine flags, because those fundamentally can lower a multi-def inst more than once, which goes against the whole premise of #9510. We just have to always materialize flags in that case. And per earlier discussions, peephole can't solve it (because elab can put other insts "in the middle") so we have no other solution to that problem.)
Fixes #14317.