Skip to content

[LiveVariables] Mark use without implicit if defined at instr #119446

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.

Sign up for GitHub

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

Merged
merged 13 commits into from
Aug 18, 2025
Merged

[LiveVariables] Mark use without implicit if defined at instr #119446

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
6b852f9
[LiveVariables] Mark use as implicit-def if def is a subregister
jofrn Dec 10, 2024
0208eb9
Rewrite loop to be modifiesRegister
jofrn Dec 11, 2024
e0cd95a
Add ll test
jofrn Dec 11, 2024
1fa40b7
Rename test, use named value & declare hidden
jofrn Dec 12, 2024
ebc5d0c
Delete PartDefRegs part of HandlePhysRegUse
jofrn Dec 18, 2024
a44bb8d
Merge branch 'main' into livevars-implicitdef
jofrn May 10, 2025
7b3266e
clang-format
jofrn May 10, 2025
a9d1662
Add tests and comments
jofrn May 10, 2025
0ecabb7
Update test after rebase
jofrn May 11, 2025
6669df1
Respond to comments
jofrn May 27, 2025
5672eae
Added FindLastPartialDef back
jofrn May 30, 2025
4ac7fce
Fix format and test
jofrn May 31, 2025
7948dfd
Update llvm/lib/CodeGen/LiveVariables.cpp
jofrn Aug 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions llvm/lib/CodeGen/LiveVariables.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,11 +277,22 @@ 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) {
Copy link
Contributor

@arsenm arsenm Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@jofrn jofrn Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

const auto MO = LastPartialDef->getOperand(I);
if (!MO.isReg() || !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*/,
true/*IsImp*/));
LastPartialDef->addOperand(
MachineOperand::CreateReg(SubReg, IsDefinedHere, true /*IsImp*/));
Copy link
Contributor

@arsenm arsenm Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@jofrn jofrn Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@arsenm arsenm Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@jofrn jofrn Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@arsenm arsenm Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@jofrn jofrn Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

PhysRegDef[SubReg] = LastPartialDef;
for (MCPhysReg SS : TRI->subregs(SubReg))
Processed.insert(SS);
Expand Down
46 changes: 46 additions & 0 deletions llvm/test/CodeGen/AMDGPU/implicitdef-subreg.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
---
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: %vval:vgpr_32 = V_MOV_B32_e32 0, implicit $exec
; CHECK-NEXT: $vgpr0 = COPY %vval
; CHECK-NEXT: $vgpr1 = COPY %vval
; CHECK-NEXT: $vgpr2 = COPY %vval
; CHECK-NEXT: $vgpr3 = COPY killed %vval, 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

%vval:vgpr_32 = V_MOV_B32_e32 0, implicit $exec

$vgpr0 = COPY %vval
$vgpr1 = COPY %vval
$vgpr2 = COPY %vval
$vgpr3 = COPY %vval
%0:vgpr_32 = COPY $vgpr0_vgpr1_vgpr2_vgpr3

...
Loading