[llvm] [WebAssembly] Fix LiveInterval verification of throwing instructions (PR #131565)
Heejin Ahn via llvm-commits
llvm-commits at lists.llvm.org
Sun Mar 16 22:51:28 PDT 2025
https://github.com/aheejin created https://github.com/llvm/llvm-project/pull/131565
This effectively reverts https://github.com/llvm/llvm-project/commit/61d22f2e4e916cdb01fd57d9145a25a5f30cc780.
https://github.com/llvm/llvm-project/commit/61d22f2e4e916cdb01fd57d9145a25a5f30cc780 claims that when the predecessor unwinds to an EH pad, we should take the live-out values of not the predecessor's terminator but the last call. But if the call has a possibility to unwind to an EH pad, it means it was an `invoke` before and it should either be the terminating instruction of a BB or be right before a branch, which is the terminator. If a call is in a middle of a BB, that means the call was not an `invoke` before and it unwinds to the caller if it throws.
This can fail in the case when there are unwinding instructions that are not calls, such as `CLEANUPRET` or other target-specific throwing instructions (e.g. Wasm's `throw`/`rethrow`/`throw_ref`). For example, as in the test case attached,
```mir
bb.2 (landing-pad):
...
CALL @foo
%0 = CONST_I32 0
CLEANUPRET %bb.3
bb.3 (landing-pad):
call @bar, %0
...
```
When examining the live range of `bb.3`'s `%0`, we should check the live-outs at `CLEANUPRET`, and not `CALL @foo`. But because of this checking routine this PR deletes, this ends up passing `CLEANUPRET` and `%0 = CONST_I32 0` and search for `%0`'s liveout at `CALL @foo`, which is incorrect and results in a validation error.
```cpp
// Predecessor of landing pad live-out on last call.
if (MFI->isEHPad()) {
for (const MachineInstr &MI : llvm::reverse(*Pred)) {
if (MI.isCall()) {
PEnd = Indexes->getInstructionIndex(MI).getBoundaryIndex();
break;
}
}
}
```
Also I'm not sure if I understand the description about `llvm.experimental.gc.statepoint` intrinsic given in https://github.com/llvm/llvm-project/commit/61d22f2e4e916cdb01fd57d9145a25a5f30cc780. There doesn't seem to be any special thing about that intrinsic; if it can unwind to an EH pad, it would be at the end of a BB. If it unwinds to the caller, it shouldn't be where we take the live-outs from. Both the test case `statepoint-invoke-ra1.ll` given in that commit, and also https://github.com/llvm/llvm-project/blob/main/llvm/test/CodeGen/X86/statepoint-invoke-ra.mir that replaced it in https://github.com/llvm/llvm-project/commit/2e1150d8aad60a8a127c10d9cd48c31334493ebf, seem to pass even with this PR (because it was an `invoke` and at the end of a BB anyway).
---
This also deletes `windows-seh-EHa-RegisterLiveness.ll`, a test case added in #76921. It looks that test case was added to track a MachineVerifier bug, which I guess may be the same thing this PR is trying to fix. After this PR, that test unexpectedly succeeds, which I think means we can delete it. (There is also another PR (#76933) that tried to fix this bug in another way, which still has not landed.)
---
This bug is discovered from the reproducer in #126916, even though this is not the bug reported there.
>From e5d0af3ab22468c314b88ebda6d9ed66d1a1098d Mon Sep 17 00:00:00 2001
From: Heejin Ahn <aheejin at gmail.com>
Date: Mon, 17 Mar 2025 04:34:39 +0000
Subject: [PATCH] [WebAssembly] Fix LiveInterval verification of throwing
instructions
This effectively reverts
https://github.com/llvm/llvm-project/commit/61d22f2e4e916cdb01fd57d9145a25a5f30cc780.
https://github.com/llvm/llvm-project/commit/61d22f2e4e916cdb01fd57d9145a25a5f30cc780
claims that when the predecessor unwinds to an EH pad, we should take
the live-out values of not the predecessor's terminator but the last
call. But if the call has a possibility to unwind to an EH pad, it means
it was an `invoke` before and it should either be the terminating
instruction of a BB or be right before a branch, which is the
terminator. If a call is in a middle of a BB, that means the call was
not an `invoke` before and it unwinds to the caller if it throws.
This can fail in the case when there are unwinding instructions that are
not calls, such as `CLEANUPRET` or other target-specific throwing
instructions (e.g. Wasm's `throw`/`rethrow`/`throw_ref`). For example,
as in the test case attached,
```mir
bb.2 (landing-pad):
...
CALL @foo
%0 = CONST_I32 0
CLEANUPRET %bb.3
bb.3 (landing-pad):
call @bar, %0
...
```
When examining the live range of `bb.3`'s `%0`, we should check the
live-outs at `CLEANUPRET`, and not `CALL @foo`. But because of this
checking routine this PR deletes, this ends up passing `CLEANUPRET` and
`%0 = CONST_I32 0` and search for `%0`'s liveout at `CALL @foo`, which
is incorrect and results in a validation error.
```cpp
// Predecessor of landing pad live-out on last call.
if (MFI->isEHPad()) {
for (const MachineInstr &MI : llvm::reverse(*Pred)) {
if (MI.isCall()) {
PEnd = Indexes->getInstructionIndex(MI).getBoundaryIndex();
break;
}
}
}
```
Also I'm not sure if I understand the description about
`llvm.experimental.gc.statepoint` intrinsic given in
https://github.com/llvm/llvm-project/commit/61d22f2e4e916cdb01fd57d9145a25a5f30cc780.
There doesn't seem to be any special thing about that intrinsic; if it
can unwind to an EH pad, it would be at the end of a BB. If it unwinds
to the caller, it shouldn't be where we take the live-outs from. Both
the test case `statepoint-invoke-ra1.ll` given in that commit, and also
https://github.com/llvm/llvm-project/blob/main/llvm/test/CodeGen/X86/statepoint-invoke-ra.mir
that replaced it in
https://github.com/llvm/llvm-project/commit/2e1150d8aad60a8a127c10d9cd48c31334493ebf,
seem to pass even with this PR (because it was an `invoke` and at the
end of a BB anyway).
---
This also deletes `windows-seh-EHa-RegisterLiveness.ll `, a test case
added in #76921. It looks that test case was added to track a
MachineVerifier bug, which I guess may be the same thing this PR is
trying to fix. After this PR, that test unexpectedly succeeds, which I
think means we can delete it. (There is also another PR (#76933) that
tried to fix this bug in another way, which still has not landed.)
---
This bug is discovered from the reproducer in #126916, even though this
is not the bug reported there.
---
llvm/lib/CodeGen/MachineVerifier.cpp | 9 ---
.../WebAssembly/cleanupret-live-intervals.mir | 48 ++++++++++++++
.../X86/windows-seh-EHa-RegisterLiveness.ll | 66 -------------------
3 files changed, 48 insertions(+), 75 deletions(-)
create mode 100644 llvm/test/CodeGen/WebAssembly/cleanupret-live-intervals.mir
delete mode 100644 llvm/test/CodeGen/X86/windows-seh-EHa-RegisterLiveness.ll
diff --git a/llvm/lib/CodeGen/MachineVerifier.cpp b/llvm/lib/CodeGen/MachineVerifier.cpp
index 87d3033038414..fa05144b152c0 100644
--- a/llvm/lib/CodeGen/MachineVerifier.cpp
+++ b/llvm/lib/CodeGen/MachineVerifier.cpp
@@ -3837,15 +3837,6 @@ void MachineVerifier::verifyLiveRangeSegment(const LiveRange &LR,
// Check that VNI is live-out of all predecessors.
for (const MachineBasicBlock *Pred : MFI->predecessors()) {
SlotIndex PEnd = LiveInts->getMBBEndIdx(Pred);
- // Predecessor of landing pad live-out on last call.
- if (MFI->isEHPad()) {
- for (const MachineInstr &MI : llvm::reverse(*Pred)) {
- if (MI.isCall()) {
- PEnd = Indexes->getInstructionIndex(MI).getBoundaryIndex();
- break;
- }
- }
- }
const VNInfo *PVNI = LR.getVNInfoBefore(PEnd);
// All predecessors must have a live-out value. However for a phi
diff --git a/llvm/test/CodeGen/WebAssembly/cleanupret-live-intervals.mir b/llvm/test/CodeGen/WebAssembly/cleanupret-live-intervals.mir
new file mode 100644
index 0000000000000..197340a4773e9
--- /dev/null
+++ b/llvm/test/CodeGen/WebAssembly/cleanupret-live-intervals.mir
@@ -0,0 +1,48 @@
+# RUN: llc -mattr=+exception-handling -wasm-enable-eh -exception-model=wasm -verify-machineinstrs -run-pass=liveintervals %s
+
+# This is a regression test for a bug where MachineVerifier compute live-outs of
+# a BB not from the end of a BB but the last call instruction, which is not
+# correct when there is another unwinding instruction after the call.
+
+--- |
+ target triple = "wasm32-unknown-unknown"
+
+ declare void @foo()
+ declare void @bar(i32)
+ define void @cleanupret_live_range_test() {
+ ret void
+ }
+...
+---
+name: cleanupret_live_range_test
+liveins:
+ - { reg: '$arguments' }
+tracksRegLiveness: true
+body: |
+ bb.0:
+ successors: %bb.1, %bb.2
+ EH_LABEL <mcsymbol .Ltmp2>
+ CALL @foo, implicit-def dead $arguments, implicit $sp32, implicit $sp64
+ EH_LABEL <mcsymbol .Ltmp3>
+ BR %bb.1, implicit-def dead $arguments
+
+ bb.1:
+ ; predecessors: %bb.0
+ UNREACHABLE implicit-def dead $arguments
+
+ bb.2 (landing-pad):
+ ; predecessors: %bb.0
+ successors: %bb.3
+ EH_LABEL <mcsymbol .Ltmp4>
+ CALL @foo, implicit-def dead $arguments, implicit $sp32, implicit $sp64
+ ; Because bb.3's use of %0's def is here, the live-outs from the predecessor
+ ; bb.2 should be from the terminator (CLEANUPRET) and not the 'CALL @foo'
+ ; above.
+ %0:i32 = CONST_I32 0, implicit-def dead $arguments
+ CLEANUPRET %bb.2, implicit-def dead $arguments
+
+ bb.3 (landing-pad):
+ ; predecessors: %bb.2
+ EH_LABEL <mcsymbol .Ltmp5>
+ CALL @bar, %0:i32, implicit-def dead $arguments, implicit $sp32, implicit $sp64
+ CLEANUPRET %bb.3, implicit-def dead $arguments
diff --git a/llvm/test/CodeGen/X86/windows-seh-EHa-RegisterLiveness.ll b/llvm/test/CodeGen/X86/windows-seh-EHa-RegisterLiveness.ll
deleted file mode 100644
index ff07f4ddf0054..0000000000000
--- a/llvm/test/CodeGen/X86/windows-seh-EHa-RegisterLiveness.ll
+++ /dev/null
@@ -1,66 +0,0 @@
-; XFAIL: *
-; RUN: llc --verify-machineinstrs < %s
-source_filename = "test.cpp"
-target datalayout = "e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
-target triple = "x86_64-pc-windows-msvc19.12.0"
-
-$"?test at Test@@Plugin@@Host@@@Z" = comdat any
-
-declare i32 @__CxxFrameHandler3(...)
-
-; Function Attrs: nounwind memory(none)
-declare void @llvm.seh.scope.begin() #1
-
-; Function Attrs: nobuiltin allocsize(0)
-declare ptr @"??2 at Test@Z"(i64) #1
-
-; Function Attrs: nounwind memory(none)
-declare void @llvm.seh.scope.end() #0
-
-; Function Attrs: nobuiltin nounwind
-declare void @"??3 at YAXPEAX@Z"(ptr) #2
-
-; Function Attrs: mustprogress uwtable
-define ptr @"?test at Test@@Plugin@@Host@@@Z"(ptr %this, ptr %host) #3 comdat align 2 personality ptr @__CxxFrameHandler3 {
-entry:
- %host.addr = alloca ptr, align 8
- %this.addr = alloca ptr, align 8
- store ptr %host, ptr %host.addr, align 8
- store ptr %this, ptr %this.addr, align 8
- %this1 = load ptr, ptr %this.addr, align 8
- %call = call noalias ptr @"??2 at Test@Z"(i64 152) #5
- invoke void @llvm.seh.scope.begin()
- to label %invoke.cont unwind label %ehcleanup
-
-invoke.cont: ; preds = %entry
- %call3 = invoke ptr @"??Test@?A0x2749C4FD@@QEAA at Test@Test@@@Z"(ptr %call, ptr %this1)
- to label %invoke.cont2 unwind label %ehcleanup
-
-invoke.cont2: ; preds = %invoke.cont
- invoke void @llvm.seh.scope.end()
- to label %invoke.cont4 unwind label %ehcleanup
-
-invoke.cont4: ; preds = %invoke.cont2
- ret ptr %call
-
-ehcleanup: ; preds = %invoke.cont2, %invoke.cont, %entry
- %0 = cleanuppad within none []
- call void @"??3 at YAXPEAX@Z"(ptr %call) #6 [ "funclet"(token %0) ]
- cleanupret from %0 unwind to caller
-}
-
-; Function Attrs: uwtable
-declare hidden ptr @"??Test@?A0x2749C4FD@@QEAA at Test@Test@@@Z"(ptr, ptr) #4 align 2
-
-attributes #0 = { nounwind memory(none) }
-attributes #1 = { nobuiltin allocsize(0) "target-cpu"="x86-64" "target-features"="+cmov,+crc32,+cx8,+fxsr,+mmx,+popcnt,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+ssse3,+x87" "tune-cpu"="generic" }
-attributes #2 = { nobuiltin nounwind "target-cpu"="x86-64" "target-features"="+cmov,+crc32,+cx8,+fxsr,+mmx,+popcnt,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+ssse3,+x87" "tune-cpu"="generic" }
-attributes #3 = { mustprogress uwtable "target-cpu"="x86-64" "target-features"="+cmov,+crc32,+cx8,+fxsr,+mmx,+popcnt,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+ssse3,+x87" "tune-cpu"="generic" }
-attributes #4 = { uwtable "target-cpu"="x86-64" "target-features"="+cmov,+crc32,+cx8,+fxsr,+mmx,+popcnt,+sse,+sse2,+sse3,+sse4.1,+sse4.2,+ssse3,+x87" "tune-cpu"="generic" }
-attributes #5 = { builtin allocsize(0) }
-attributes #6 = { builtin nounwind }
-
-!llvm.module.flags = !{!1, !2}
-
-!1 = !{i32 2, !"eh-asynch", i32 1}
-!2 = !{i32 7, !"uwtable", i32 2}
More information about the llvm-commits
mailing list