Skip to content

[AMDGPU][MachineRegisterInfo] Extend the MRI live-ins check to account for Subreg #126926

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

Closed
wants to merge 4 commits into from
Closed

[AMDGPU][MachineRegisterInfo] Extend the MRI live-ins check to account for Subreg #126926

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
447eb33
[AMDGPU][MachineRegisterInfo] Extend the MRI live-ins check by including
vg0204 Feb 12, 2025
e384137
Modified MRI liviIn check to have unconditional subreg check
vg0204 Feb 13, 2025
df4bf0b
updated the pre-commit tests resolved via the extended MRI liveIns ch…
vg0204 Feb 25, 2025
7b07a1a
Replaced subreg check with regUnits check in MRI isLiveIn
vg0204 Feb 25, 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
14 changes: 13 additions & 1 deletion llvm/lib/CodeGen/MachineRegisterInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,9 +448,21 @@ void MachineRegisterInfo::clearKillFlags(Register Reg) const {
}

bool MachineRegisterInfo::isLiveIn(Register Reg) const {
Copy link
Contributor

Choose a reason for hiding this comment

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

What other users of this exist? I'd prefer to see a patch to SILowerSGPRSpills to not use the MRI liveins before we do anything here.

Copy link
Contributor

Choose a reason for hiding this comment

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

To elaborate, I do not believe MachineRegisterInfo's live in tracking to be generally reliable. GlobalISel doesn't bother adding entries to it for instance. The verifier does not check that it is consistent with the entry block's live in list. Last I looked at it, I thought we could possibly remove the concept entirely.

I think we need to establish a policy on the live in entry : function live ins to see if it makes sense to have liveness tracking. Right now it's only kind of useful in argument lowering for initial codegen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What other users of this exist? I'd prefer to see a patch to SILowerSGPRSpills to not use the MRI liveins before we do anything here.

It requires LiveRegUnitSet (set of all live regUnits) concept in MBB, and its propagation to all its live-ins related APIs to achieve it. As current isLiveIn is based on exact match of PhysReg (which would discard the idea of subReg check entirely). Can we proceed with this idea!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can it be achieved using LiveRegUnit ??

Copy link
Contributor

Choose a reason for hiding this comment

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

It requires LiveRegUnitSet (set of all live regUnits) concept in MBB, and its propagation to all its live-ins related APIs to achieve it.

I don't follow. We don't need any liveness tracking to check if a physical register is live into the function. Why can't you just s/MRI.isLiveIn(Reg)/MF.begin()->isLiveIn(Reg)/?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't follow. We don't need any liveness tracking to check if a physical register is live into the function. Why can't you just s/MRI.isLiveIn(Reg)/MF.begin()->isLiveIn(Reg)/?

Because as the current MBB's isLiveIn defined it won't work as expected to get rid of the issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kindly look into #129847 and #129848, with implementation changes needed to utilize MBB's isLiveIn function!

for (const std::pair<MCRegister, Register> &LI : liveins())
for (const std::pair<MCRegister, Register> &LI : liveins()) {
if ((Register)LI.first == Reg || LI.second == Reg)
return true;

// Check if Reg is a subreg of live-in register
MCRegister PhysReg = LI.first;
if (!PhysReg.isValid() || !Reg.isPhysical())
continue;

const TargetRegisterInfo *TRI = getTargetRegisterInfo();
if (all_of(TRI->regunits(Reg), [&](const MCRegUnit RegUnit) {
return llvm::is_contained(TRI->regunits(PhysReg), RegUnit);
}))
return true;
}
return false;
}

Expand Down
38 changes: 17 additions & 21 deletions llvm/test/CodeGen/AMDGPU/spill-partial-csr-sgpr-live-ins.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,25 +1,6 @@
# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
# RUN: not --crash llc -mtriple=amdgcn-amd-amdhsa -verify-machineinstrs -run-pass=si-lower-sgpr-spills %s -o /dev/null 2>&1 | FileCheck -check-prefix=VERIFIER %s
# RUN: llc -mtriple=amdgcn-amd-amdhsa -verify-machineinstrs -run-pass=si-lower-sgpr-spills -o - %s| FileCheck %s

# FIXME : Currently, MRI's liveIn check for registers does not take the corresponding live-in's sub-registers into account. As a result
# in SILowerSGPRSpills, the SubReg spill gets marked KILLED even though its SuperReg is in the function Live-ins. This causes machine
# verifier to now fail at direct usage of that SubReg, which intially should not be any problem before adding spill.

# VERIFIER: After SI lower SGPR spill instructions

# VERIFIER: *** Bad machine code: Using an undefined physical register ***
# VERIFIER: - instruction: S_NOP 0, implicit $sgpr50
# VERIFIER-NEXT: - operand 1: implicit $sgpr50

# VERIFIER: *** Bad machine code: Using an undefined physical register ***
# VERIFIER: - instruction: S_NOP 0, implicit $sgpr52
# VERIFIER-NEXT: - operand 1: implicit $sgpr52

# VERIFIER: *** Bad machine code: Using an undefined physical register ***
# VERIFIER: - instruction: S_NOP 0, implicit $sgpr55
# VERIFIER-NEXT: - operand 1: implicit $sgpr55

# VERIFIER: LLVM ERROR: Found 3 machine code errors.
---
name: spill_partial_live_csr_sgpr_test
tracksRegLiveness: true
Expand All @@ -30,7 +11,22 @@ liveins:
body: |
bb.0:
liveins: $sgpr50_sgpr51, $sgpr52_sgpr53, $sgpr54_sgpr55

; CHECK-LABEL: name: spill_partial_live_csr_sgpr_test
; CHECK: liveins: $sgpr50, $sgpr52, $sgpr53, $sgpr54, $sgpr55, $sgpr56, $vgpr63, $sgpr50_sgpr51, $sgpr52_sgpr53, $sgpr54_sgpr55
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: $vgpr63 = SI_SPILL_S32_TO_VGPR $sgpr50, 0, $vgpr63
; CHECK-NEXT: $vgpr63 = SI_SPILL_S32_TO_VGPR $sgpr52, 1, $vgpr63
; CHECK-NEXT: $vgpr63 = SI_SPILL_S32_TO_VGPR $sgpr53, 2, $vgpr63
; CHECK-NEXT: $vgpr63 = SI_SPILL_S32_TO_VGPR $sgpr54, 3, $vgpr63
; CHECK-NEXT: $vgpr63 = SI_SPILL_S32_TO_VGPR $sgpr55, 4, $vgpr63
; CHECK-NEXT: $vgpr63 = SI_SPILL_S32_TO_VGPR killed $sgpr56, 5, $vgpr63
; CHECK-NEXT: S_NOP 0, implicit $sgpr50
; CHECK-NEXT: $sgpr50 = S_MOV_B32 0
; CHECK-NEXT: S_NOP 0, implicit $sgpr52
; CHECK-NEXT: $sgpr52_sgpr53 = S_MOV_B64 0
; CHECK-NEXT: S_NOP 0, implicit $sgpr55
; CHECK-NEXT: $sgpr54_sgpr55 = S_MOV_B64 0
; CHECK-NEXT: $sgpr56 = S_MOV_B32 0
S_NOP 0, implicit $sgpr50
$sgpr50 = S_MOV_B32 0
S_NOP 0, implicit $sgpr52
Expand Down