Skip to content

[LiveVariables] Mark use as implicit-def 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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+77 −37
Open

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 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
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
8 changes: 5 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,13 @@ void LiveVariables::HandlePhysRegUse(Register Reg, MachineInstr &MI) {
continue;
if (PartDefRegs.count(SubReg))
continue;

// Check if SubReg is defined at LastPartialDef.
bool IsDefinedHere = LastPartialDef->modifiesRegister(SubReg, TRI);
// 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

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

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

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

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
2 changes: 1 addition & 1 deletion llvm/test/CodeGen/AArch64/ldrpre-ldr-merge.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Contributor

Choose a reason for hiding this comment

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

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

; CHECK-NEXT: STPXi renamable $x0, renamable $x2, renamable $x1, 0 :: (store (s64))
; CHECK-NEXT: RET undef $lr
early-clobber renamable $x1, renamable $x0 = LDRSWpre killed renamable $x1, 40 :: (load (s32))
Expand Down
28 changes: 28 additions & 0 deletions llvm/test/CodeGen/AMDGPU/fncall-implicitdef.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
; RUN: llc -mtriple=amdgcn -mcpu=gfx900 -O1 %s -o - | FileCheck %s

define amdgpu_ps <4 x float> @caller(ptr %1) {
; CHECK-LABEL: caller:
; CHECK: ; %bb.0:
; CHECK-NEXT: flat_load_dword v1, v[0:1]
; CHECK-NEXT: s_getpc_b64 s[0:1]
; CHECK-NEXT: s_add_u32 s0, s0, fn@gotpcrel32@lo+4
; CHECK-NEXT: s_addc_u32 s1, s1, fn@gotpcrel32@hi+12
; CHECK-NEXT: s_load_dwordx2 s[4:5], s[0:1], 0x0
; CHECK-NEXT: s_mov_b32 s0, 0
; CHECK-NEXT: s_mov_b32 s1, 0
; CHECK-NEXT: s_mov_b32 s2, 0
; CHECK-NEXT: s_mov_b64 s[8:9], 36
; CHECK-NEXT: v_mov_b32_e32 v0, 0
; CHECK-NEXT: s_mov_b32 s3, 0
; CHECK-NEXT: v_mov_b32_e32 v2, 0
; CHECK-NEXT: s_mov_b32 s32, 0
; CHECK-NEXT: s_waitcnt lgkmcnt(0)
; CHECK-NEXT: s_swappc_b64 s[30:31], s[4:5]
; CHECK-NEXT: ; return to shader part epilog
%L = load i32, ptr %1, align 4
%R = call <4 x float> @fn(<4 x i32> zeroinitializer, i32 0, i32 %L, i32 0)
ret <4 x float> %R
}

declare <4 x float> @fn(<4 x i32> inreg, i32, i32, i32)
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-def $sgpr0, implicit-def $sgpr1, implicit-def $sgpr2, implicit-def $sgpr0_sgpr1, implicit-def $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-def $vgpr0, implicit-def $vgpr1, implicit-def $vgpr2, implicit-def $vgpr0_vgpr1, implicit-def $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