Cranelift: resolve value aliases when printing CLIF functions#8214
Merged
cfallin merged 1 commit intobytecodealliance:mainfrom Mar 22, 2024
Merged
Conversation
This makes the CLIF much easier to follow.
fitzgen
requested review from
alexcrichton and
cfallin
and removed request for
a team
github-actions
bot
added
the
cranelift
cfallin
approved these changes
Mar 22, 2024
Member
cfallin
left a comment
cfallin
left a comment
There was a problem hiding this comment.
A long-overdue ease of use fix -- thanks a bunch!
jameysharp
added a commit
to jameysharp/wasmtime
that referenced
this pull request
Mar 22, 2024
This is a follow-up to bytecodealliance#8214. That PR made the CLIF printer resolve aliases before printing any value, which means now the aliases are never referenced. I expected plenty of tests to change due to removing the aliases from the printed representation of CLIF functions, but there was one surprise: in tests/disas/icall-loop.wat, this didn't just remove the aliases, but also changed the value numbering. As it turns out, in that one function, the last value in the function that Wasmtime generated was an alias (v28 -> v3). The disas test harness has Wasmtime print the CLIF it generated, then the harness re-parses that CLIF, and passes the re-parsed CLIF to Cranelift's optimization passes. Since after this change the aliases are not printed, re-parsing the printed form doesn't round-trip perfectly, and specifically the last allocated value number changes. As a result, when legalization and optimization introduce new instructions, they get different result value numbers. My conclusion is that this is fine. The disas tests don't need to produce exactly the same value numbers that the full Wasmtime to Cranelift pipeline would produce. This process does produce the same instructions in the same order, and I think that's all that matters.
jameysharp
added a commit
to jameysharp/wasmtime
that referenced
this pull request
Mar 23, 2024
This reverts commit 6c51848, "Cranelift: resolve value aliases when printing CLIF functions (bytecodealliance#8214)" In discussion on bytecodealliance#8223, we decided to handle this a different way. So I've introduced a method on DataFlowGraph which eliminates all value aliases, and we can call it when necessary. In this PR, I've specifically called it in disas tests. In these tests, value aliases are just clutter that distracts from understanding what code will actually be generated. They may change due to changes in legalization or optimization, but that doesn't signal anything about whatever the test was intended to check. There are other places we would probably be better off doing a whole-function rewrite once rather than calling `resolve_aliases` a bunch of times, such as the egraph pass and lowering. I have not changed those in this PR.
jameysharp
added a commit
to jameysharp/wasmtime
that referenced
this pull request
Mar 26, 2024
This reverts commit 6c51848, "Cranelift: resolve value aliases when printing CLIF functions (bytecodealliance#8214)", then applies the same effect a different way. In discussion on bytecodealliance#8223, we decided to handle this a different way. So I've introduced a method on DataFlowGraph which eliminates all value aliases, and we can call it when necessary. If that function is not called then the CLIF printer should print value aliases the same way it did before bytecodealliance#8214. In this PR, I've specifically called it in disas tests. The changes to write.rs and frontend.rs are from the revert, while the changes to disas.rs are new. In these tests, value aliases are just clutter that distracts from understanding what code will actually be generated. They may change due to changes in legalization, but that doesn't signal anything about whatever the test was intended to check. Because the new helper deletes aliases after it's done resolving them, those aliases now disappear from the test expectations. Aside from that, the test expectations are unchanged compared to what bytecodealliance#8214 did.
jameysharp
added a commit
to jameysharp/wasmtime
that referenced
this pull request
Mar 26, 2024
This reverts commit 6c51848, "Cranelift: resolve value aliases when printing CLIF functions (bytecodealliance#8214)", then applies the same effect a different way. In discussion on bytecodealliance#8223, we decided to handle this a different way. So I've introduced a method on DataFlowGraph which eliminates all value aliases, and we can call it when necessary. If that function is not called then the CLIF printer should print value aliases the same way it did before bytecodealliance#8214. In this PR, I've specifically called it in disas tests. The changes to write.rs and frontend.rs are from the revert, while the changes to disas.rs are new. In these tests, value aliases are just clutter that distracts from understanding what code will actually be generated. They may change due to changes in legalization, but that doesn't signal anything about whatever the test was intended to check. Because the new helper deletes aliases after it's done resolving them, those aliases now disappear from the test expectations. Aside from that, the test expectations are unchanged compared to what bytecodealliance#8214 did.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Mar 26, 2024
This reverts commit 6c51848, "Cranelift: resolve value aliases when printing CLIF functions (#8214)", then applies the same effect a different way. In discussion on #8223, we decided to handle this a different way. So I've introduced a method on DataFlowGraph which eliminates all value aliases, and we can call it when necessary. If that function is not called then the CLIF printer should print value aliases the same way it did before #8214. In this PR, I've specifically called it in disas tests. The changes to write.rs and frontend.rs are from the revert, while the changes to disas.rs are new. In these tests, value aliases are just clutter that distracts from understanding what code will actually be generated. They may change due to changes in legalization, but that doesn't signal anything about whatever the test was intended to check. Because the new helper deletes aliases after it's done resolving them, those aliases now disappear from the test expectations. Aside from that, the test expectations are unchanged compared to what #8214 did.
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
2 participants
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 makes the CLIF much easier to follow.