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

vg0204 · 2025-02-12T15:38:15Z

This support extends live-in check for MRI, by adding check for sub-register checks also. It solves a BUG related to SILowerSGPRSpill, where KILL flag is introduced based on MRI live-in list. Now, if a SuperReg is in live-ins, it implies its immediate usage as well as any subpart of it. But, currently, only exact register match is taken care by MRI, bypassing the immediately utilized subregs as not used, thus marking it as Killed, and later machine verfier starts giving error for using killed subreg.

Refer #126696 for more details.

llvmbot · 2025-02-12T15:38:45Z

@llvm/pr-subscribers-backend-amdgpu

Author: Vikash Gupta (vg0204)

Changes

This support extends live-in check for MRI, by adding check for sub-register checks also. It solves a BUG related to SILowerSGPRSpill, where KILL flag is introduced based on MRI live-in list. Now, if a SuperReg is in live-ins, it implies its immediate usage as well as any subpart of it. But, currently, only exact register match is taken care by MRI, bypassing the immediately utilized subregs as not used, thus marking it as Killed, and later machine verfier starts giving error for using killed subreg.

Refer #126696 for more details.

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

2 Files Affected:

(modified) llvm/include/llvm/CodeGen/MachineRegisterInfo.h (+1-1)
(modified) llvm/lib/CodeGen/MachineRegisterInfo.cpp (+14-2)

diff --git a/llvm/include/llvm/CodeGen/MachineRegisterInfo.h b/llvm/include/llvm/CodeGen/MachineRegisterInfo.h
index 1c465741cb462..f182b42d2a24a 100644
--- a/llvm/include/llvm/CodeGen/MachineRegisterInfo.h
+++ b/llvm/include/llvm/CodeGen/MachineRegisterInfo.h
@@ -1020,7 +1020,7 @@ class MachineRegisterInfo {
     return LiveIns;
   }
 
-  bool isLiveIn(Register Reg) const;
+  bool isLiveIn(Register Reg, bool CheckForSubreg = false) const;
 
   /// getLiveInPhysReg - If VReg is a live-in virtual register, return the
   /// corresponding live-in physical register.
diff --git a/llvm/lib/CodeGen/MachineRegisterInfo.cpp b/llvm/lib/CodeGen/MachineRegisterInfo.cpp
index 937f63f6c5e00..341b0c7207092 100644
--- a/llvm/lib/CodeGen/MachineRegisterInfo.cpp
+++ b/llvm/lib/CodeGen/MachineRegisterInfo.cpp
@@ -447,10 +447,22 @@ void MachineRegisterInfo::clearKillFlags(Register Reg) const {
     MO.setIsKill(false);
 }
 
-bool MachineRegisterInfo::isLiveIn(Register Reg) const {
-  for (const std::pair<MCRegister, Register> &LI : liveins())
+bool MachineRegisterInfo::isLiveIn(Register Reg, bool CheckForSubreg) const {
+  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
+    if (CheckForSubreg) {
+      MCRegister PhysReg = LI.first;
+      if (!PhysReg.isValid())
+        continue;
+
+      for (MCPhysReg SubReg : getTargetRegisterInfo()->subregs(PhysReg))
+        if (SubReg == Reg)
+          return true;
+    }
+  }
   return false;
 }

arsenm · 2025-02-12T16:53:12Z

llvm/include/llvm/CodeGen/MachineRegisterInfo.h

@@ -1020,7 +1020,7 @@ class MachineRegisterInfo {
    return LiveIns;
  }

-  bool isLiveIn(Register Reg) const;
+  bool isLiveIn(Register Reg, bool CheckForSubreg = false) const;


I think that if we do this, it should not be an option. It should simply be unconditional.

However, I think we need better documented rules for whatever is supposed to be in the livein list. The MachineFunction liveins, for whatever reason, behave differently from MachineBasicBlock's liveins. Why don't they both use the physreg + lane mask?

However, I still think we should replace all of the livein references to be directly in terms of regunits

However, I still think we should replace all of the livein references to be directly in terms of regunits

Makes sense to me, but it would quite a huge task, as live-ins appeared & represented differently across different places need to be handled :

In MBB, live-ins are stored as physreg + lane mask (regunit based implementation)

In MachinFunction, live-ins are just pair of <PhysReg, VirtualReg> in association

In MachineVerfier, MBB's live-ins are calculated as the physReg & all its subregs

I think that if we do this, it should not be an option. It should simply be unconditional.

Need to see its side-effect on other architectures, as can't comment on their interpretation right! For now, its needed as immediate fix to mentioned crashing mir test.

Done with it!

Makes sense to me, but it would quite a huge task, as live-ins appeared & represented differently across different places need to be handled :

In MBB, live-ins are stored as physreg + lane mask (regunit based implementation)
In MachinFunction, live-ins are just pair of <PhysReg, VirtualReg> in association
In MachineVerfier, MBB's live-ins are calculated as the physReg & all its subregs

We should eventually port them to use regunits.

Done it already for MachineFunction here itself, only apparently seen it left for machine verifier, which will require bit of work, so better to do separately. Any other place @cdevadas @arsenm you think it should be taken care. If its OK with you, I can take it up this task, atleast the supposedly apparent things!

That can be done separately.

cdevadas · 2025-02-14T09:38:07Z

This should be a stacked review based of #126696 so that we can see how this patch changes the test.

vg0204 · 2025-02-19T08:41:23Z

@arsenm kind Ping!

arsenm

On second look, I'm not sure what problem this is supposed to solve. Last I checked, the MachineVerifier did not use the MachineRegisterInfo / MachineFunction liveins for anything. It does not factor into the liveness calculation, or verification. Only the MachineBasicBlock's liveins should matter (where we do have actual bugs in the verifier, especially for the entry block).

arsenm · 2025-02-19T10:40:56Z

SILowerSGPRSpill, where KILL flag is introduced based on MRI live-in list.

Can this look at the block live ins instead?

vg0204 · 2025-02-20T06:41:52Z

On second look, I'm not sure what problem this is supposed to solve. Last I checked, the MachineVerifier did not use the MachineRegisterInfo / MachineFunction liveins for anything. It does not factor into the liveness calculation, or verification. Only the MachineBasicBlock's liveins should matter (where we do have actual bugs in the verifier, especially for the entry block).

Yes, thats true and as you said looking into MBB(entry) live-in should apprently take care of machine verifier error spawned in SILowerSgprSpills(). But, if we see from other point of view considering your comments

However, I still think we should replace all of the livein references to be directly in terms of regunits

Don't we really need this!

vg0204 · 2025-02-20T09:58:19Z

SILowerSGPRSpill, where KILL flag is introduced based on MRI live-in list.

Can this look at the block live ins instead?

block live hold T(physReg+LaneBitmask) as liveIns, so its check is meant to handle only liveIns existing as T, while in SILowerSGPR needs check for CSR physReg used i.e. dword-sized reg as liveIn. So, straight direct check without anything additional not seems possible. With MRI, it holds only the entire live PhysReg as MCRegister in its list, so just its extension by my patch does the job.

the check against the subregs for live-ins.

…eck.

vg0204 · 2025-02-25T09:39:36Z

Updated the PR with pre-commited test changes (rebased from upstream) for final review @arsenm , @cdevadas , @jayfoad !

vg0204 · 2025-03-04T06:15:21Z

Gentle Ping @arsenm , @jayfoad , @cdevadas !

arsenm · 2025-03-04T06:17:30Z

llvm/lib/CodeGen/MachineRegisterInfo.cpp

@@ -448,9 +448,21 @@ void MachineRegisterInfo::clearKillFlags(Register Reg) const {
 }

 bool MachineRegisterInfo::isLiveIn(Register Reg) const {


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.

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

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!

Can it be achieved using LiveRegUnit ??

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)/?

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

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

…terator in SILowerSGPRSpills (#129848) This patch replaces use of MachineRegisterInfo's liveIn check with the machine basicBlock's liveIn. As the MRI's liveIn is inconsistent with the entry MBB liveIns, when it comes to the machine verifier checks. PS: Its an alternative solution with respect to #126926.

vg0204 · 2025-03-18T05:23:46Z

The alternative way to handle it via usage of MCRegAliasIterator has been merged!

vg0204 added backend:AMDGPU llvm:codegen labels Feb 12, 2025

vg0204 requested review from jayfoad, arsenm and cdevadas February 12, 2025 15:38

vg0204 self-assigned this Feb 12, 2025

arsenm reviewed Feb 12, 2025

View reviewed changes

vg0204 marked this pull request as ready for review February 13, 2025 07:19

vg0204 requested a review from arsenm February 14, 2025 09:34

vg0204 requested review from RamNalamothu and pravinjagtap February 17, 2025 12:06

arsenm reviewed Feb 19, 2025

View reviewed changes

cdevadas mentioned this pull request Feb 24, 2025

[AMDGPU][NFC] Added test for live-in CSR SGPR used partially giving MachineVerifier error #126696

Merged

vg0204 added 3 commits February 25, 2025 14:34

[AMDGPU][MachineRegisterInfo] Extend the MRI live-ins check by including

447eb33

the check against the subregs for live-ins.

Modified MRI liviIn check to have unconditional subreg check

e384137

updated the pre-commit tests resolved via the extended MRI liveIns ch…

df4bf0b

…eck.

vg0204 force-pushed the vg0204/extend-MRI-liveInCheck-siLowerSgprSpill branch from c38bcb7 to df4bf0b Compare February 25, 2025 09:27

Replaced subreg check with regUnits check in MRI isLiveIn

7b07a1a

vg0204 requested a review from arsenm February 26, 2025 12:06

arsenm reviewed Mar 4, 2025

View reviewed changes

vg0204 requested a review from MatzeB March 5, 2025 08:33

vg0204 mentioned this pull request Mar 5, 2025

[AMDGPU][CodeGen] Using MBB's liveIn check in tandem with MCRegAliasIterator in SILowerSGPRSpills #129848

Merged

vg0204 closed this Mar 18, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

vg0204 commented Feb 12, 2025

llvmbot commented Feb 12, 2025

arsenm Feb 12, 2025

vg0204 Feb 12, 2025

vg0204 Feb 12, 2025 •

edited

Loading

vg0204 Feb 13, 2025

cdevadas Feb 14, 2025

vg0204 Feb 14, 2025

cdevadas Feb 14, 2025

cdevadas commented Feb 14, 2025

vg0204 commented Feb 19, 2025

arsenm left a comment

arsenm commented Feb 19, 2025

vg0204 commented Feb 20, 2025 •

edited

Loading

vg0204 commented Feb 20, 2025

vg0204 commented Feb 25, 2025 •

edited

Loading

vg0204 commented Mar 4, 2025

arsenm Mar 4, 2025

arsenm Mar 4, 2025

vg0204 Mar 4, 2025

vg0204 Mar 4, 2025

arsenm Mar 4, 2025

vg0204 Mar 5, 2025

vg0204 Mar 5, 2025

vg0204 commented Mar 18, 2025

		@@ -448,9 +448,21 @@ void MachineRegisterInfo::clearKillFlags(Register Reg) const {
		}

		bool MachineRegisterInfo::isLiveIn(Register Reg) const {

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

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

Conversation

vg0204 commented Feb 12, 2025

llvmbot commented Feb 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vg0204 Feb 12, 2025 • 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

Choose a reason for hiding this comment

cdevadas commented Feb 14, 2025

vg0204 commented Feb 19, 2025

arsenm left a comment

Choose a reason for hiding this comment

arsenm commented Feb 19, 2025

vg0204 commented Feb 20, 2025 • edited Loading

vg0204 commented Feb 20, 2025

vg0204 commented Feb 25, 2025 • edited Loading

vg0204 commented Mar 4, 2025

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vg0204 commented Mar 18, 2025

vg0204 Feb 12, 2025 •

edited

Loading

vg0204 commented Feb 20, 2025 •

edited

Loading

vg0204 commented Feb 25, 2025 •

edited

Loading