-
Notifications
You must be signed in to change notification settings - Fork 14.8k
[WebAssembly] Unstackify registers with no uses in ExplicitLocals #149626
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
There are cases we end up removing some intructions that use stackified registers after RegStackify. For example, ```wasm bb.0: %0 = ... ;; %0 is stackified br_if %bb.1, %0 bb.1: ``` In this code, br_if will be removed in CFGSort, so we should unstackify %0 so that it can be correctly dropped in ExplicitLocals. Rather than handling this in case-by-case basis, this PR just unstackifies all stackifies register with no uses, so that they can be correctly dropped. Fixes llvm#149097.
@llvm/pr-subscribers-backend-webassembly Author: Heejin Ahn (aheejin) ChangesThere are cases we end up removing some intructions that use stackified registers after RegStackify. For example, bb.0:
%0 = ... ;; %0 is stackified
br_if %bb.1, %0
bb.1: In this code, br_if will be removed in CFGSort, so we should unstackify %0 so that it can be correctly dropped in ExplicitLocals. Rather than handling this in case-by-case basis, this PR just unstackifies all stackifies register with no uses, so that they can be correctly dropped. Fixes #149097. Full diff: https://github.com/llvm/llvm-project/pull/149626.diff 2 Files Affected:
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyExplicitLocals.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyExplicitLocals.cpp
index 2662241ef8499..ad28e36f29112 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyExplicitLocals.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyExplicitLocals.cpp
@@ -256,9 +256,17 @@ bool WebAssemblyExplicitLocals::runOnMachineFunction(MachineFunction &MF) {
// Precompute the set of registers that are unused, so that we can insert
// drops to their defs.
+ // And unstackify any stackified registers that don't have any uses, so that
+ // they can be dropped later. This can happen when transformations after
+ // RegStackify removes instructions that use stackified registers.
BitVector UseEmpty(MRI.getNumVirtRegs());
- for (unsigned I = 0, E = MRI.getNumVirtRegs(); I < E; ++I)
- UseEmpty[I] = MRI.use_empty(Register::index2VirtReg(I));
+ for (unsigned I = 0, E = MRI.getNumVirtRegs(); I < E; ++I) {
+ Register Reg = Register::index2VirtReg(I);
+ if (MRI.use_empty(Reg)) {
+ UseEmpty[I] = true;
+ MFI.unstackifyVReg(Reg);
+ }
+ }
// Visit each instruction in the function.
for (MachineBasicBlock &MBB : MF) {
diff --git a/llvm/test/CodeGen/WebAssembly/removed-terminator.ll b/llvm/test/CodeGen/WebAssembly/removed-terminator.ll
new file mode 100644
index 0000000000000..8c9b6ff4bca97
--- /dev/null
+++ b/llvm/test/CodeGen/WebAssembly/removed-terminator.ll
@@ -0,0 +1,14 @@
+; RUN: llc -O0 < %s
+
+target triple = "wasm32-unknown-unknown"
+
+define void @test(i1 %x) {
+ %y = xor i1 %x, true
+ ; This br_if's operand (%y) is stackified in RegStackify. But this terminator
+ ; will be removed in CFGSort after that. We need to make sure we unstackify %y
+ ; so that it can be dropped in ExplicitLocals.
+ br i1 %y, label %exit, label %exit
+
+exit:
+ ret void
+}
|
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.
LGTM. I confirmed that this also fixes my original issue.
/cherry-pick b13bca7 |
/pull-request #150187 |
…vm#149626) There are cases we end up removing some intructions that use stackified registers after RegStackify. For example, ```wasm bb.0: %0 = ... ;; %0 is stackified br_if %bb.1, %0 bb.1: ``` In this code, br_if will be removed in CFGSort, so we should unstackify %0 so that it can be correctly dropped in ExplicitLocals. Rather than handling this in case-by-case basis, this PR just unstackifies all stackifies register with no uses in the beginning of ExplicitLocals, so that they can be correctly dropped. Fixes llvm#149097. (cherry picked from commit b13bca7)
…vm#149626) There are cases we end up removing some intructions that use stackified registers after RegStackify. For example, ```wasm bb.0: %0 = ... ;; %0 is stackified br_if %bb.1, %0 bb.1: ``` In this code, br_if will be removed in CFGSort, so we should unstackify %0 so that it can be correctly dropped in ExplicitLocals. Rather than handling this in case-by-case basis, this PR just unstackifies all stackifies register with no uses in the beginning of ExplicitLocals, so that they can be correctly dropped. Fixes llvm#149097.
`FAKE_USE`s are essentially no-ops, so they have to be removed before running ExplicitLocals so that `drop`s will be correctly inserted to drop those values used by the `FAKE_USE`s. --- This is reapplication of llvm#160228, which broke Wasm waterfall. This PR additionally prevents `FAKE_USE`s uses from being stackified. Previously, a 'def' whose first use was a `FAKE_USE` was able to be stackified as `TEE`: - Before ``` Reg = INST ... // Def FAKE_USE ..., Reg, ... // Insert INST ..., Reg, ... INST ..., Reg, ... ``` - After RegStackify ``` DefReg = INST ... // Def TeeReg, Reg = TEE ... DefReg FAKE_USE ..., TeeReg, ... // Insert INST ..., Reg, ... INST ..., Reg, ... ``` And this assumes `DefReg` and `TeeReg` are stackified. But this PR removes `FAKE_USE`s in the beginning of ExplicitLocals. And later in ExplicitLocals we have a routine to unstackify registers that have no uses left: https://github.com/llvm/llvm-project/blob/7b28fcd2b182ba2c9d2d71c386be92fc0ee3cc9d/llvm/lib/Target/WebAssembly/WebAssemblyExplicitLocals.cpp#L257-L269 (This was added in llvm#149626. Then it didn't seem it would trigger the same assertions for `TEE`s because it was fixing the bug where a terminator was removed in CFGSort (llvm#149097). Details here: llvm#149432 (comment)) - After `FAKE_USE` removal and unstackification ``` DefReg = INST ... TeeReg, Reg = TEE ... DefReg INST ..., Reg, ... INST ..., Reg, ... ``` And now `TeeReg` is unstackified. This triggered the assertion here, that `TeeReg` should be stackified: https://github.com/llvm/llvm-project/blob/7b28fcd2b182ba2c9d2d71c386be92fc0ee3cc9d/llvm/lib/Target/WebAssembly/WebAssemblyExplicitLocals.cpp#L316 This prevents `FAKE_USE`s' uses from being stackified altogether, including `TEE` transformation. Even when it is not a `TEE` transformation and just a single use stackification, it does not trigger the assertion but there's no point stackifying it given that it will be deleted. --- Fixes emscripten-core/emscripten#25301.
`FAKE_USE`s are essentially no-ops, so they have to be removed before running ExplicitLocals so that `drop`s will be correctly inserted to drop those values used by the `FAKE_USE`s. --- This is reapplication of #160228, which broke Wasm waterfall. This PR additionally prevents `FAKE_USE`s uses from being stackified. Previously, a 'def' whose first use was a `FAKE_USE` was able to be stackified as `TEE`: - Before ``` Reg = INST ... // Def FAKE_USE ..., Reg, ... // Insert INST ..., Reg, ... INST ..., Reg, ... ``` - After RegStackify ``` DefReg = INST ... // Def TeeReg, Reg = TEE ... DefReg FAKE_USE ..., TeeReg, ... // Insert INST ..., Reg, ... INST ..., Reg, ... ``` And this assumes `DefReg` and `TeeReg` are stackified. But this PR removes `FAKE_USE`s in the beginning of ExplicitLocals. And later in ExplicitLocals we have a routine to unstackify registers that have no uses left: https://github.com/llvm/llvm-project/blob/7b28fcd2b182ba2c9d2d71c386be92fc0ee3cc9d/llvm/lib/Target/WebAssembly/WebAssemblyExplicitLocals.cpp#L257-L269 (This was added in #149626. Then it didn't seem it would trigger the same assertions for `TEE`s because it was fixing the bug where a terminator was removed in CFGSort (#149097). Details here: #149432 (comment)) - After `FAKE_USE` removal and unstackification ``` DefReg = INST ... TeeReg, Reg = TEE ... DefReg INST ..., Reg, ... INST ..., Reg, ... ``` And now `TeeReg` is unstackified. This triggered the assertion here, that `TeeReg` should be stackified: https://github.com/llvm/llvm-project/blob/7b28fcd2b182ba2c9d2d71c386be92fc0ee3cc9d/llvm/lib/Target/WebAssembly/WebAssemblyExplicitLocals.cpp#L316 This prevents `FAKE_USE`s' uses from being stackified altogether, including `TEE` transformation. Even when it is not a `TEE` transformation and just a single use stackification, it does not trigger the assertion but there's no point stackifying it given that it will be deleted. --- Fixes emscripten-core/emscripten#25301.
`FAKE_USE`s are essentially no-ops, so they have to be removed before running ExplicitLocals so that `drop`s will be correctly inserted to drop those values used by the `FAKE_USE`s. --- This is reapplication of llvm#160228, which broke Wasm waterfall. This PR additionally prevents `FAKE_USE`s uses from being stackified. Previously, a 'def' whose first use was a `FAKE_USE` was able to be stackified as `TEE`: - Before ``` Reg = INST ... // Def FAKE_USE ..., Reg, ... // Insert INST ..., Reg, ... INST ..., Reg, ... ``` - After RegStackify ``` DefReg = INST ... // Def TeeReg, Reg = TEE ... DefReg FAKE_USE ..., TeeReg, ... // Insert INST ..., Reg, ... INST ..., Reg, ... ``` And this assumes `DefReg` and `TeeReg` are stackified. But this PR removes `FAKE_USE`s in the beginning of ExplicitLocals. And later in ExplicitLocals we have a routine to unstackify registers that have no uses left: https://github.com/llvm/llvm-project/blob/7b28fcd2b182ba2c9d2d71c386be92fc0ee3cc9d/llvm/lib/Target/WebAssembly/WebAssemblyExplicitLocals.cpp#L257-L269 (This was added in llvm#149626. Then it didn't seem it would trigger the same assertions for `TEE`s because it was fixing the bug where a terminator was removed in CFGSort (llvm#149097). Details here: llvm#149432 (comment)) - After `FAKE_USE` removal and unstackification ``` DefReg = INST ... TeeReg, Reg = TEE ... DefReg INST ..., Reg, ... INST ..., Reg, ... ``` And now `TeeReg` is unstackified. This triggered the assertion here, that `TeeReg` should be stackified: https://github.com/llvm/llvm-project/blob/7b28fcd2b182ba2c9d2d71c386be92fc0ee3cc9d/llvm/lib/Target/WebAssembly/WebAssemblyExplicitLocals.cpp#L316 This prevents `FAKE_USE`s' uses from being stackified altogether, including `TEE` transformation. Even when it is not a `TEE` transformation and just a single use stackification, it does not trigger the assertion but there's no point stackifying it given that it will be deleted. --- Fixes emscripten-core/emscripten#25301.
There are cases we end up removing some intructions that use stackified registers after RegStackify. For example,
In this code, br_if will be removed in CFGSort, so we should unstackify %0 so that it can be correctly dropped in ExplicitLocals.
Rather than handling this in case-by-case basis, this PR just unstackifies all stackifies register with no uses in the beginning of ExplicitLocals, so that they can be correctly dropped.
Fixes #149097.