-
Notifications
You must be signed in to change notification settings - Fork 15.5k
release/21.x: [WebAssembly] Remove FAKE_USEs before ExplicitLocals (#160768) #171184
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
|
@dschuff What do you think about merging this PR to the release branch? |
|
@llvm/pr-subscribers-backend-webassembly Author: None (llvmbot) ChangesBackport e5b2a06 Requested by: @dschuff 3 Files Affected:
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyExplicitLocals.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyExplicitLocals.cpp
index e6486e247209b..5c3127e2d3dc6 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyExplicitLocals.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyExplicitLocals.cpp
@@ -216,6 +216,18 @@ static MachineInstr *findStartOfTree(MachineOperand &MO,
return Def;
}
+// FAKE_USEs are no-ops, so remove them here so that the values used by them
+// will be correctly dropped later.
+static void removeFakeUses(MachineFunction &MF) {
+ SmallVector<MachineInstr *> ToDelete;
+ for (auto &MBB : MF)
+ for (auto &MI : MBB)
+ if (MI.isFakeUse())
+ ToDelete.push_back(&MI);
+ for (auto *MI : ToDelete)
+ MI->eraseFromParent();
+}
+
bool WebAssemblyExplicitLocals::runOnMachineFunction(MachineFunction &MF) {
LLVM_DEBUG(dbgs() << "********** Make Locals Explicit **********\n"
"********** Function: "
@@ -226,6 +238,8 @@ bool WebAssemblyExplicitLocals::runOnMachineFunction(MachineFunction &MF) {
WebAssemblyFunctionInfo &MFI = *MF.getInfo<WebAssemblyFunctionInfo>();
const auto *TII = MF.getSubtarget<WebAssemblySubtarget>().getInstrInfo();
+ removeFakeUses(MF);
+
// Map non-stackified virtual registers to their local ids.
DenseMap<unsigned, unsigned> Reg2Local;
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
index bc91c6424b63e..fd13ef9a1921d 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
@@ -866,6 +866,10 @@ bool WebAssemblyRegStackify::runOnMachineFunction(MachineFunction &MF) {
if (Insert->isDebugValue())
continue;
+ // Ignore FAKE_USEs, which are no-ops and will be deleted later.
+ if (Insert->isFakeUse())
+ continue;
+
// Iterate through the inputs in reverse order, since we'll be pulling
// operands off the stack in LIFO order.
CommutingState Commuting;
diff --git a/llvm/test/CodeGen/WebAssembly/fake-use.ll b/llvm/test/CodeGen/WebAssembly/fake-use.ll
new file mode 100644
index 0000000000000..a18ce33566df0
--- /dev/null
+++ b/llvm/test/CodeGen/WebAssembly/fake-use.ll
@@ -0,0 +1,25 @@
+; RUN: llc < %s | llvm-mc -triple=wasm32-unknown-unknown
+
+target triple = "wasm32-unknown-unknown"
+
+define void @fake_use() {
+ %t = call i32 @foo()
+ tail call void (...) @llvm.fake.use(i32 %t)
+ ret void
+}
+
+; %t shouldn't be converted to TEE in RegStackify, because the FAKE_USE will be
+; deleted in the beginning of ExplicitLocals.
+define void @fake_use_no_tee() {
+ %t = call i32 @foo()
+ tail call void (...) @llvm.fake.use(i32 %t)
+ call void @use(i32 %t)
+ ret void
+}
+
+declare i32 @foo()
+declare void @use(i32 %t)
+; Function Attrs: mustprogress nocallback nofree nosync nounwind willreturn memory(inaccessiblemem: readwrite)
+declare void @llvm.fake.use(...) #0
+
+attributes #0 = { mustprogress nocallback nofree nosync nounwind willreturn memory(inaccessiblemem: readwrite) }
|
|
This patch fixes a regression bug from the previous LLVM version and maintains API and ABI compatibility, so I think it qualifies for a release branch patch. |
`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. (cherry picked from commit e5b2a06)
|
@dschuff (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR. |
Backport e5b2a06
Requested by: @dschuff