[LiveVariables] Mark use as implicit-def if defined at instr #119446

jofrn · 2024-12-10T19:58:26Z

LiveVariables will mark instructions with their implicit subregister uses. However, it will miss marking the subregister as an implicit-def if its own definition is a subregister of it, i.e. $r3 = OP val, implicit-def $r0_r1_r2_r3, ..., implicit $r2_r3, which defines $r3 on the same line it is used.

This change ensures such uses are marked as implicit-def, i.e. $r3 = OP val, implicit-def $r0_r1_r2_r3, ..., implicit-def $r2_r3.

llvmbot · 2024-12-10T19:58:57Z

@llvm/pr-subscribers-backend-aarch64
@llvm/pr-subscribers-llvm-regalloc

@llvm/pr-subscribers-backend-amdgpu

Author: None (jofrn)

Changes

LiveVariables will mark instructions with their implicit subregister uses. However, it will miss marking the subregister as an implicit-def if its own definition is a subregister of it, i.e. $r3 = OP val, implicit-def $r0_r1_r2_r3, ..., implicit $r2_r3, which defines $sr3 on the same line it is used.

This change ensures such uses are marked as implicit-def, i.e. $r3 = OP val, implicit-def $r0_r1_r2_r3, ..., implicit-def $r2_r3.

Full diff: https://github.com/llvm/llvm-project/pull/119446.diff

2 Files Affected:

(modified) llvm/lib/CodeGen/LiveVariables.cpp (+12-1)
(added) llvm/test/CodeGen/AMDGPU/implicitdef-subreg.mir (+44)

diff --git a/llvm/lib/CodeGen/LiveVariables.cpp b/llvm/lib/CodeGen/LiveVariables.cpp
index f17d60dc22dda9..e6753563a24c28 100644
--- a/llvm/lib/CodeGen/LiveVariables.cpp
+++ b/llvm/lib/CodeGen/LiveVariables.cpp
@@ -277,10 +277,21 @@ void LiveVariables::HandlePhysRegUse(Register Reg, MachineInstr &MI) {
           continue;
         if (PartDefRegs.count(SubReg))
           continue;
+
+        // Check if SubReg is defined at LastPartialDef.
+        bool IsDefinedHere = false;
+        for (int i = 0; i < LastPartialDef->getNumOperands(); ++i) {
+          auto MO = LastPartialDef->getOperand(i);
+          if (!MO.isDef()) continue;
+          if (TRI->isSubRegister(SubReg, MO.getReg())) {
+            IsDefinedHere = true;
+            break;
+          }
+        }
         // This part of Reg was defined before the last partial def. It's killed
         // here.
         LastPartialDef->addOperand(MachineOperand::CreateReg(SubReg,
-                                                             false/*IsDef*/,
+                                                             IsDefinedHere/*IsDef*/,
                                                              true/*IsImp*/));
         PhysRegDef[SubReg] = LastPartialDef;
         for (MCPhysReg SS : TRI->subregs(SubReg))
diff --git a/llvm/test/CodeGen/AMDGPU/implicitdef-subreg.mir b/llvm/test/CodeGen/AMDGPU/implicitdef-subreg.mir
new file mode 100644
index 00000000000000..9b11bb482a028e
--- /dev/null
+++ b/llvm/test/CodeGen/AMDGPU/implicitdef-subreg.mir
@@ -0,0 +1,44 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 5
+# RUN: llc -mtriple=amdgcn -mcpu=gfx1010 -O1 %s --run-pass=livevars -implicitdef -o - | FileCheck %s
+---
+name:            sgpr_copy
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    ; CHECK-LABEL: name: sgpr_copy
+    ; CHECK: %sval:sreg_32 = S_MOV_B32 0
+    ; CHECK-NEXT: $sgpr0 = COPY %sval
+    ; CHECK-NEXT: $sgpr1 = COPY %sval
+    ; CHECK-NEXT: $sgpr2 = COPY %sval
+    ; CHECK-NEXT: $sgpr3 = COPY killed %sval, implicit-def $sgpr0_sgpr1_sgpr2_sgpr3, implicit $sgpr0, implicit $sgpr1, implicit $sgpr2, implicit $sgpr0_sgpr1, implicit $sgpr0_sgpr1_sgpr2, implicit-def $sgpr2_sgpr3
+    ; CHECK-NEXT: dead $sgpr30_sgpr31 = COPY killed $sgpr0_sgpr1_sgpr2_sgpr3
+    %sval:sreg_32 = S_MOV_B32 0
+
+    $sgpr0 = COPY %sval
+    $sgpr1 = COPY %sval
+    $sgpr2 = COPY %sval
+    $sgpr3 = COPY %sval
+    $sgpr30_sgpr31 = COPY $sgpr0_sgpr1_sgpr2_sgpr3
+
+...
+---
+name:            vgpr_copy
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    ; CHECK-LABEL: name: vgpr_copy
+    ; CHECK: %sval:vgpr_32 = V_MOV_B32_e32 0, implicit $exec
+    ; CHECK-NEXT: $vgpr0 = COPY %sval
+    ; CHECK-NEXT: $vgpr1 = COPY %sval
+    ; CHECK-NEXT: $vgpr2 = COPY %sval
+    ; CHECK-NEXT: $vgpr3 = COPY killed %sval, implicit-def $vgpr0_vgpr1_vgpr2_vgpr3, implicit $vgpr0, implicit $vgpr1, implicit $vgpr2, implicit $vgpr0_vgpr1, implicit $vgpr0_vgpr1_vgpr2, implicit-def $vgpr1_vgpr2_vgpr3
+    ; CHECK-NEXT: dead [[COPY:%[0-9]+]]:vgpr_32 = COPY killed $vgpr0_vgpr1_vgpr2_vgpr3
+    %sval:vgpr_32 = V_MOV_B32_e32 0, implicit $exec
+
+    $vgpr0 = COPY %sval
+    $vgpr1 = COPY %sval
+    $vgpr2 = COPY %sval
+    $vgpr3 = COPY %sval
+    %0:vgpr_32 = COPY $vgpr0_vgpr1_vgpr2_vgpr3
+
+...

github-actions · 2024-12-10T20:01:45Z

✅ With the latest revision this PR passed the C/C++ code formatter.

llvm/lib/CodeGen/LiveVariables.cpp

LiveVariables will mark instructions with their implicit subregister uses. However, it will miss marking the subregister as an implicit-def if its own definition is a subregister of it, i.e. `$r3 = OP val, implicit-def $r0_r1_r2_r3, ..., implicit $r2_r3`, which defines $sr3 on the same line it is used. This change ensures such uses are marked as implicit-def, i.e. `$r3 = OP val, implicit-def $r0_r1_r2_r3, ..., implicit-def $r2_r3`.

arsenm

What do you need this for? We should be trying to get rid of the LiveVariables pass. All of the code here for handling sub registers is very out of date

arsenm · 2024-12-11T03:31:02Z

llvm/lib/CodeGen/LiveVariables.cpp

+
+        // Check if SubReg is defined at LastPartialDef.
+        bool IsDefinedHere = false;
+        for (int I = 0; I < LastPartialDef->getNumOperands(); ++I) {


This whole function looks like it could use a rewrite. Is this a re-invention of MachineInstr::modifiesRegister?

The parent function, LiveVariables::HandlePhysRegUse, will additionally collect definitions of Reg (e.g. four COPY instructions for a use of $sgpr0_$sgpr1_$sgpr2_sgpr3 in a subsequent instruction), and mark the last definition with its implicit uses and defs.

modifiesRegister can be used here in place of this loop. It will also mark superregisters though, so will try the tests again...

jofrn · 2024-12-11T09:28:36Z

What do you need this for? We should be trying to get rid of the LiveVariables pass. All of the code here for handling sub registers is very out of date

If we do not have this, then LiveRangeCalc::findReachingDefs may fail:

Use of $sgpr2_sgpr3 does not have a corresponding definition on every path:
248r $sgpr3 = S_MOV_B32 0, implicit-def $sgpr0_sgpr1_sgpr2_sgpr3, implicit $sgpr0, implicit $sgpr1, implicit $sgpr2, implicit $sgpr0_sgpr1, implicit $sgpr0_sgpr1_sgpr2, implicit $sgpr2_sgpr3
LLVM ERROR: Use not jointly dominated by defs.

arsenm · 2024-12-11T09:39:02Z

If we do not have this, then LiveRangeCalc::findReachingDefs may fail:

Use of $sgpr2_sgpr3 does not have a corresponding definition on every path:
248r $sgpr3 = S_MOV_B32 0, implicit-def $sgpr0_sgpr1_sgpr2_sgpr3, implicit $sgpr0, implicit $sgpr1, implicit $sgpr2, implicit $sgpr0_sgpr1, implicit $sgpr0_sgpr1_sgpr2, implicit $sgpr2_sgpr3
LLVM ERROR: Use not jointly dominated by defs.

Do you have an IR sample that hits this? Should add as a test if so

This also causes superregisters to be marked as implicit-def.

jofrn · 2024-12-11T20:09:23Z

Do you have an IR sample that hits this? Should add as a test if so

Yes, added it in.

llvm/test/CodeGen/AMDGPU/fncall-implicitdef.ll

llvm/test/CodeGen/AMDGPU/implicitdef-subreg.mir

llvm/test/CodeGen/AMDGPU/fncall-implicitdef.ll

arsenm · 2024-12-12T11:16:32Z

llvm/lib/CodeGen/LiveVariables.cpp

-                                                             false/*IsDef*/,
-                                                             true/*IsImp*/));
+        LastPartialDef->addOperand(
+            MachineOperand::CreateReg(SubReg, IsDefinedHere, true /*IsImp*/));


Swapping what used to be a use with a def sounds wrong. Do we need multiple implicit operands to model this liveness?

It will only be switched if the use is defined at this line. By the way, if the use is omitted or marked as implicit-def, the test will also pass, so we do not need implicit-def ; we just need to remove the implicit use.

Multiple implicit operands will be added if they are subregs of Reg within LiveVariables::HandlePhysRegUse, so I believe it is already being modeled.

Alternatively, there can potentially be handling of this issue at the point of the error (during LiveIntervalCalc). The implicit use causes a "use not jointly dominated by def" error here.

we just need to remove the implicit use.

So can you just skip the add operand altogether? I'm still confused about why this pass is doing anything. This is very old code that predates subregister tracking. I bet we can delete all of this code as a step towards deleting the entire pass

Yes, it can be skipped. However, llvm/test/CodeGen/AArch64/ldrpre-ldr-merge.mir would need to be updated to not mark implicit-def $w2_hi, which may be incorrect, so I believe it is better to leave the implicit-def in.

I'd argue that we are not really replacing what used to be a use with a def. They should have been defs all along because the sub or super register has a definition at this line.

If we do not fix it here, the logic to fix it in LiveRangeCalc.cpp needs to update LiveRangeCalc::findReachingDefs to take into consideration definitions that are found at the current instruction. And it propagates a change to be made in RegisterPressure.cpp, so I think it is cleaner to have it here.

LiveRangeCalc requiring this pass run before is spaghetti. We shouldn't need this pass to fix the liveness representation. The liveness of physical registers should be fully expressed at all points in the compilation. This pass has been papering over issues in LiveRangeCalc

The implicit-def of w2_hi is redundant with the x2 implicit-def. The use was marking a kill (except it didn't add a kill flag?). So there's no reason to have the new implicit-def, and losing the implicit use loses the use kill point

Removed a chunk of the code, which also got rid of w2_hi, so now it doesn't have a kill point... Need to take a closer look at it to see if we can keep w2_hi here. Perhaps it doesn't have a kill flag for the reason of it being erroneously added in this function.

arsenm · 2024-12-17T02:26:22Z

llvm/test/CodeGen/AArch64/ldrpre-ldr-merge.mir

@@ -756,7 +756,7 @@ body:             |
    ; CHECK: liveins: $x0, $x1, $x2
    ; CHECK-NEXT: {{  $}}
    ; CHECK-NEXT: early-clobber renamable $x1, renamable $x0 = LDRSWpre renamable $x1, 40, implicit $w1, implicit $w1_hi :: (load (s32))
-    ; CHECK-NEXT: renamable $w2 = LDRWui renamable $x1, 1, implicit-def $x2, implicit $w2_hi :: (load (s32))
+    ; CHECK-NEXT: renamable $w2 = LDRWui renamable $x1, 1, implicit-def $x2, implicit-def $w2_hi :: (load (s32))


x2 is composed of [w2, and w2_hi], the implicit-def of w2_hi is redundant with the implicit-def of $x2.

arsenm · 2024-12-17T02:31:53Z

llvm/lib/CodeGen/LiveVariables.cpp

-                                                             false/*IsDef*/,
-                                                             true/*IsImp*/));
+        LastPartialDef->addOperand(
+            MachineOperand::CreateReg(SubReg, IsDefinedHere, true /*IsImp*/));


LiveRangeCalc requiring this pass run before is spaghetti. We shouldn't need this pass to fix the liveness representation. The liveness of physical registers should be fully expressed at all points in the compilation. This pass has been papering over issues in LiveRangeCalc

The implicit-def of w2_hi is redundant with the x2 implicit-def. The use was marking a kill (except it didn't add a kill flag?). So there's no reason to have the new implicit-def, and losing the implicit use loses the use kill point

sgundapa · 2025-04-21T14:31:07Z

Any thing else that needs to be addressed other than the lit test ?

arsenm

I guess we can push forward with this, but we should still work to delete this pass. This is really out of date subregister management, and LiveRangeCalc should not require preprocessing

arsenm · 2025-04-23T11:21:56Z

llvm/test/CodeGen/AMDGPU/livevars-implicitdef.mir

@@ -0,0 +1,46 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 5
+# RUN: llc -mtriple=amdgcn --run-pass=livevars -o - %s | FileCheck %s
+---


Comment the purpose of the test

arsenm · 2025-04-23T11:23:25Z

llvm/test/CodeGen/AMDGPU/livevars-implicitdef.mir

+    $vgpr2 = COPY %vval
+    $vgpr3 = COPY %vval
+    %0:vgpr_32 = COPY $vgpr0_vgpr1_vgpr2_vgpr3
+


Add some more complex cases, like a def of a different tuple in the middle (e.g. v0, v[1:2], v3). And cases where the result has undefined holes (e.g. def v0, v2, v3 leaving v1 undef)

jofrn requested review from arsenm, shiltian and Pierre-vh December 10, 2024 19:58

llvmbot added backend:AMDGPU llvm:regalloc labels Dec 10, 2024

shiltian reviewed Dec 10, 2024

View reviewed changes

llvm/lib/CodeGen/LiveVariables.cpp Outdated Show resolved Hide resolved

shiltian reviewed Dec 10, 2024

View reviewed changes

llvm/lib/CodeGen/LiveVariables.cpp Outdated Show resolved Hide resolved

jofrn force-pushed the livevars-implicitdef branch 5 times, most recently from bb287c4 to d450a15 Compare December 11, 2024 01:31

jofrn force-pushed the livevars-implicitdef branch from d450a15 to 6b852f9 Compare December 11, 2024 01:32

arsenm reviewed Dec 11, 2024

View reviewed changes

Rewrite loop to be modifiesRegister

0208eb9

This also causes superregisters to be marked as implicit-def.

jofrn force-pushed the livevars-implicitdef branch from 9c0aa9b to 0208eb9 Compare December 11, 2024 10:41

llvmbot added the backend:AArch64 label Dec 11, 2024

jofrn changed the title ~~[LiveVariables] Mark use as implicit-def if def is a subregister~~ [LiveVariables] Mark use as implicit-def if defined at instr Dec 11, 2024

Add ll test

e0cd95a

arsenm reviewed Dec 12, 2024

View reviewed changes

Rename test, use named value & declare hidden

1fa40b7

arsenm reviewed Dec 17, 2024

View reviewed changes

jofrn force-pushed the livevars-implicitdef branch from 86d785f to b65b610 Compare December 18, 2024 10:32

Delete PartDefRegs part of HandlePhysRegUse

ebc5d0c

jofrn force-pushed the livevars-implicitdef branch from b65b610 to ebc5d0c Compare December 18, 2024 10:42

jofrn requested review from arsenm and shiltian December 18, 2024 11:53

arsenm reviewed Apr 23, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[LiveVariables] Mark use as implicit-def if defined at instr #119446

[LiveVariables] Mark use as implicit-def if defined at instr #119446

jofrn commented Dec 10, 2024 •

edited

Loading

llvmbot commented Dec 10, 2024 •

edited

Loading

github-actions bot commented Dec 10, 2024 •

edited

Loading

arsenm left a comment

arsenm Dec 11, 2024

jofrn Dec 11, 2024 •

edited

Loading

jofrn commented Dec 11, 2024

arsenm commented Dec 11, 2024

jofrn commented Dec 11, 2024

arsenm Dec 12, 2024

jofrn Dec 12, 2024 •

edited

Loading

arsenm Dec 13, 2024

jofrn Dec 17, 2024

arsenm Dec 17, 2024

jofrn Dec 18, 2024 •

edited

Loading

arsenm Dec 17, 2024

arsenm Dec 17, 2024

sgundapa commented Apr 21, 2025 •

edited

Loading

arsenm left a comment

arsenm Apr 23, 2025

arsenm Apr 23, 2025

[LiveVariables] Mark use as implicit-def if defined at instr #119446

Are you sure you want to change the base?

[LiveVariables] Mark use as implicit-def if defined at instr #119446

Conversation

jofrn commented Dec 10, 2024 • edited Loading

llvmbot commented Dec 10, 2024 • edited Loading

github-actions bot commented Dec 10, 2024 • edited Loading

arsenm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jofrn Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

jofrn commented Dec 11, 2024

arsenm commented Dec 11, 2024

jofrn commented Dec 11, 2024

Choose a reason for hiding this comment

jofrn Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jofrn Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgundapa commented Apr 21, 2025 • edited Loading

arsenm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jofrn commented Dec 10, 2024 •

edited

Loading

llvmbot commented Dec 10, 2024 •

edited

Loading

github-actions bot commented Dec 10, 2024 •

edited

Loading

jofrn Dec 11, 2024 •

edited

Loading

jofrn Dec 12, 2024 •

edited

Loading

jofrn Dec 18, 2024 •

edited

Loading

sgundapa commented Apr 21, 2025 •

edited

Loading