[WebAssembly] Split separate component LiveIntervals for TEEs #131561
Conversation
`MachineVerifier` requires that a virtual register's `LiveInterval` has only one connected component: https://github.com/llvm/llvm-project/blob/f4043f451d0e8c30c8a9826ce87a6e76f3ace468/llvm/lib/CodeGen/MachineVerifier.cpp#L3923-L3936 (This is different from SSA; there can be multiple defs for a register, but those live intervals should somehow be _connected_. I am not very sure why this rule exists, but this rule apparently has been there forever since llvm@260fa28.) --- And it turns out our `TEE` generation in RegStackify can create virtual registers with `LiveInterval` with multiple disconnected segments. This is how `TEE` generation works: https://github.com/llvm/llvm-project/blob/f4043f451d0e8c30c8a9826ce87a6e76f3ace468/llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp#L613-L632 But it is possible that `Reg = INST` can also use `Reg`: ``` 0 Reg = ... ... 1 Reg = INST ..., Reg, ... // It both defs and uses 'Reg' 2 INST ..., Reg, ... 3 INST ..., Reg, ... 4 INST ..., Reg, ... ``` In this pseudocode, `Reg`'s live interval is `[0r,1r),[1r:4r)`, which has two segments that are connected. But after the `TEE` transformation, ``` 0 Reg = ... ... 1 DefReg = INST ..., Reg, ... 2 TeeReg, Reg = TEE_... DefReg 3 INST ..., TeeReg, ... 4 INST ..., Reg, ... 5 INST ..., Reg, ... ``` now `%0`'s live interval is `[0r,1r),[2r,4r)`, which are not connected anymore. In many cases, these split segments are connected by another segment generated by a `PHI` (or a block start boundary that used to be a `PHI`). For example, if there is a loop, ``` bb.0: successors: %bb.1 0 Reg = INST ... ... bb.1: ; predecessors: %bb.1 successors: %bb.1, %bb.2 1 DefReg = INST ..., Reg, ... 2 TeeReg, Reg = TEE_... DefReg 3 INST ..., TeeReg, ... 4 INST ..., Reg, ... 5 INST ..., Reg, ... 6 BR_IF bb.1 bb.2: ; predecessors: %bb.1 7 INST ... ``` The live interval would be `[0r,1B),[1B,1r),[2r,7B)` and these three segments are classified as a single equivalence class because of the segment `[1B,1r)` starting at the merging point (which used to be a `PHI`) at the start of bb.1. But this kind of connection is not always guaranteed. The method that determines whether all components are connected, i.e., there is a single equivalence class in a `LiveRange`, is `ConnectedVNInfoEqClasses::Classify`. --- In RegStackify, there is a routine that splits live intervals into multiple registers in some cases: https://github.com/llvm/llvm-project/blob/f4043f451d0e8c30c8a9826ce87a6e76f3ace468/llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp#L511-L517 It looks this was copied from https://github.com/llvm/llvm-project/blob/dc9a183ac6aa2d087ceac56970255b06c4772ca3/llvm/lib/CodeGen/RegisterCoalescer.cpp#L341-L353. But `LiveIntervals::shrinkToUses` does not return true for all unconnected live intervals. I don't understand all the details of that function or why `RegisterCoalescer::shrinkToUses` was written that way, but it looks it returns true when there are some dead components: https://github.com/llvm/llvm-project/blob/926d980017d82dedb9eb50147a82fdfb01659f16/llvm/lib/CodeGen/LiveIntervals.cpp#L537-L540 And the case in the attached test case does not return true here, but still has multiple unconnected components and does not pass `MachineVerifier` unless they are split. --- So this PR runs `LiveIntervals::splitSeparateComponents` regardless of the return value of `LiveIntervals::shrinkToUses`. `splitSeparateComponents` won't do anything unless there are multiple unconnected components to split. --- This is one of the bugs reported in llvm#126916.
|
@llvm/pr-subscribers-backend-webassembly Author: Heejin Ahn (aheejin) Changes
llvm-project/llvm/lib/CodeGen/MachineVerifier.cpp Lines 3923 to 3936 in f4043f4 260fa28.) And it turns out our This is how llvm-project/llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp Lines 613 to 632 in f4043f4 But it is possible that In this pseudocode, now In many cases, these split segments are connected by another segment generated by a The live interval would be llvm-project/llvm/lib/CodeGen/LiveInterval.cpp Lines 1329 to 1369 in de03e10 In RegStackify, there is a routine that splits live intervals into multiple registers in some cases: llvm-project/llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp Lines 511 to 517 in f4043f4 llvm-project/llvm/lib/CodeGen/RegisterCoalescer.cpp Lines 341 to 353 in dc9a183 But llvm-project/llvm/lib/CodeGen/LiveIntervals.cpp Lines 537 to 540 in 926d980 MachineVerifier unless they are split.
So this PR runs This is one of the bugs reported in #126916. Full diff: https://github.com/llvm/llvm-project/pull/131561.diff 2 Files Affected:
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
index 428d573668fb4..5ff3d5f760204 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
@@ -510,10 +510,11 @@ static unsigned getTeeOpcode(const TargetRegisterClass *RC) {
// Shrink LI to its uses, cleaning up LI.
static void shrinkToUses(LiveInterval &LI, LiveIntervals &LIS) {
- if (LIS.shrinkToUses(&LI)) {
- SmallVector<LiveInterval *, 4> SplitLIs;
- LIS.splitSeparateComponents(LI, SplitLIs);
- }
+ LIS.shrinkToUses(&LI);
+ // In case the register's live interval now has multiple unconnected
+ // components, split them into multiple registers.
+ SmallVector<LiveInterval *, 4> SplitLIs;
+ LIS.splitSeparateComponents(LI, SplitLIs);
}
/// A single-use def in the same block with no intervening memory or register
diff --git a/llvm/test/CodeGen/WebAssembly/tee-live-intervals.mir b/llvm/test/CodeGen/WebAssembly/tee-live-intervals.mir
new file mode 100644
index 0000000000000..b137e990932b0
--- /dev/null
+++ b/llvm/test/CodeGen/WebAssembly/tee-live-intervals.mir
@@ -0,0 +1,41 @@
+# RUN: llc -mtriple=wasm32-unknown-unknown -run-pass wasm-reg-stackify -verify-machineinstrs %s -o -
+
+# TEE generation in RegStackify can create virtual registers with LiveIntervals
+# with multiple disconnected segments, which is invalid in MachineVerifier. In
+# this test, '%0 = CALL @foo' will become a CALL and a TEE, which creates
+# unconnected split segments. This should be later split into multiple
+# registers. This test should not crash with -verify-machineinstrs, which checks
+# whether all segments within a register is connected. See ??? for the detailed
+# explanation.
+
+--- |
+ target triple = "wasm32-unknown-unknown"
+
+ declare ptr @foo(ptr returned)
+ define void @tee_live_intervals_test() {
+ ret void
+ }
+...
+---
+name: tee_live_intervals_test
+liveins:
+ - { reg: '$arguments' }
+tracksRegLiveness: true
+body: |
+ bb.0:
+ liveins: $arguments
+ successors: %bb.1, %bb.2
+ %0:i32 = ARGUMENT_i32 0, implicit $arguments
+ %1:i32 = CONST_I32 0, implicit-def dead $arguments
+ BR_IF %bb.2, %1:i32, implicit-def dead $arguments
+
+ bb.1:
+ ; predecessors: %bb.0
+ %0:i32 = CALL @foo, %0:i32, implicit-def dead $arguments, implicit $sp32, implicit $sp64
+ STORE8_I32_A32 0, 0, %0:i32, %1:i32, implicit-def dead $arguments
+ RETURN %0:i32, implicit-def dead $arguments
+
+ bb.2:
+ ; predecessors: %bb.0
+ %2:i32 = CONST_I32 0, implicit-def dead $arguments
+ RETURN %2:i32, implicit-def dead $arguments
|
|
Your analysis here sounds sensible to me, but I also don't understand all the details of why |
|
Yeah I don't know why |
|
@qcolombet I think llvm-project/llvm/lib/CodeGen/RegisterCoalescer.cpp Lines 341 to 353 in 9d92d4b was first written in 0ddd315 authored by you. It's a long time ago, but in case you remember the context, could you explain what this condition means, and why it does not return true in some cases when there are unconnected components (as described in the test case of this PR)? |
If this is not the case, then there may be a bug in
I'm wondering if you're trying to apply the same recipe as the register coalescer on something that doesn't follow the same rules. Could you check (e.g., by inserting calls to the verifier at different stages of the reg stackifier) whether your live intervals are "correct" when you enter into |
|
@qcolombet Thanks for the info; I'll take a look. Meanwhile, do you know why this verification rule, which requires a llvm-project/llvm/lib/CodeGen/MachineVerifier.cpp Lines 3923 to 3936 in f4043f4 I haven't read the implementation of https://github.com/llvm/llvm-project/blob/main/llvm/lib/CodeGen/RegisterCoalescer.cpp, but in general what register coalescing does is to reuse registers if live ranges don't overlap, which I think naturally create multiple unconnected components. For example, In this pseudocode, where But this can create unconnected live intervals for |
That's because you don't want to group variables that are otherwise completely unrelated. That creates additional constraints on your allocation for no reason. |
Sorry, not sure if I understand. Then doesn't this effectively ban register coalescing, as I asked in #131561 (comment)? |
In a way it does, because these two variables are unrelated, so there's no point in coalescing them. |
|
Let me rephrase, it bans coalescing of unrelated variables. |
MachineVerifierrequires that a virtual register'sLiveIntervalhas only one connected component:llvm-project/llvm/lib/CodeGen/MachineVerifier.cpp
Lines 3923 to 3936 in f4043f4
And it turns out our
TEEgeneration in RegStackify can create virtual registers withLiveIntervalwith multiple disconnected segments.This is how
TEEgeneration works:llvm-project/llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
Lines 613 to 632 in f4043f4
But it is possible that
Reg = INSTcan also useReg:In this pseudocode,
Reg's live interval is[0r,1r),[1r:4r), which has two segments that are connected. But after theTEEtransformation,now
%0's live interval is[0r,1r),[2r,4r), which are not connected anymore.In many cases, these split segments are connected by another segment generated by a
PHI(or a block start boundary that used to be aPHI). For example, if there is a loop,The live interval would be
[0r,1B),[1B,1r),[2r,7B)and these three segments are classified as a single equivalence class because of the segment[1B,1r)starting at the merging point (which used to be aPHI) at the start of bb.1. But this kind of connection is not always guaranteed. The method that determines whether all components are connected, i.e., there is a single equivalence class in aLiveRange, isConnectedVNInfoEqClasses::Classify.In RegStackify, there is a routine that splits live intervals into multiple registers in some cases:
llvm-project/llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
Lines 511 to 517 in f4043f4
llvm-project/llvm/lib/CodeGen/RegisterCoalescer.cpp
Lines 341 to 353 in dc9a183
But
LiveIntervals::shrinkToUsesdoes not return true for all unconnected live intervals. I don't understand all the details of that function or whyRegisterCoalescer::shrinkToUseswas written that way, but it looks it returns true when there are some dead components:llvm-project/llvm/lib/CodeGen/LiveIntervals.cpp
Lines 537 to 540 in 926d980
MachineVerifierunless they are split.So this PR runs
LiveIntervals::splitSeparateComponentsregardless of the return value ofLiveIntervals::shrinkToUses.splitSeparateComponentswon't do anything unless there are multiple unconnected components to split.This is one of the bugs reported in #126916.