[PHIElimination] Handle subranges in LiveInterval updates #69429

perlfu · 2023-10-18T07:57:18Z

Add subrange tracking and handling for LiveIntervals during PHI elimination.
This requires extending MachineBasicBlock::SplitCriticalEdge to also update subrange intervals.

llvmbot · 2023-10-18T07:58:26Z

@llvm/pr-subscribers-llvm-regalloc

@llvm/pr-subscribers-backend-amdgpu

Author: Carl Ritson (perlfu)

Changes

Add handling for subrange updates in LiveInterval preservation.

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

5 Files Affected:

(modified) llvm/include/llvm/CodeGen/LiveIntervals.h (+4)
(modified) llvm/lib/CodeGen/LiveIntervals.cpp (+1-1)
(modified) llvm/lib/CodeGen/MachineBasicBlock.cpp (+12)
(modified) llvm/lib/CodeGen/PHIElimination.cpp (+48-18)
(modified) llvm/test/CodeGen/AMDGPU/split-mbb-lis-subrange.mir (+15-7)

diff --git a/llvm/include/llvm/CodeGen/LiveIntervals.h b/llvm/include/llvm/CodeGen/LiveIntervals.h
index 903e458854f6cd0..e0344c2dbd0abe3 100644
--- a/llvm/include/llvm/CodeGen/LiveIntervals.h
+++ b/llvm/include/llvm/CodeGen/LiveIntervals.h
@@ -139,6 +139,10 @@ class VirtRegMap;
       return LI;
     }
 
+    LiveInterval &getOrCreateEmptyInterval(Register Reg) {
+      return hasInterval(Reg) ? getInterval(Reg) : createEmptyInterval(Reg);
+    }
+
     /// Interval removal.
     void removeInterval(Register Reg) {
       delete VirtRegIntervals[Reg];
diff --git a/llvm/lib/CodeGen/LiveIntervals.cpp b/llvm/lib/CodeGen/LiveIntervals.cpp
index da55e7f7284b364..70fd881ad85b57e 100644
--- a/llvm/lib/CodeGen/LiveIntervals.cpp
+++ b/llvm/lib/CodeGen/LiveIntervals.cpp
@@ -862,7 +862,7 @@ float LiveIntervals::getSpillWeight(bool isDef, bool isUse,
 
 LiveRange::Segment
 LiveIntervals::addSegmentToEndOfBlock(Register Reg, MachineInstr &startInst) {
-  LiveInterval &Interval = createEmptyInterval(Reg);
+  LiveInterval &Interval = getOrCreateEmptyInterval(Reg);
   VNInfo *VN = Interval.getNextValue(
       SlotIndex(getInstructionIndex(startInst).getRegSlot()),
       getVNInfoAllocator());
diff --git a/llvm/lib/CodeGen/MachineBasicBlock.cpp b/llvm/lib/CodeGen/MachineBasicBlock.cpp
index 14d9bb292ddf2e8..4961cd234608775 100644
--- a/llvm/lib/CodeGen/MachineBasicBlock.cpp
+++ b/llvm/lib/CodeGen/MachineBasicBlock.cpp
@@ -1275,6 +1275,8 @@ MachineBasicBlock *MachineBasicBlock::SplitCriticalEdge(
           assert(VNI &&
                  "PHI sources should be live out of their predecessors.");
           LI.addSegment(LiveInterval::Segment(StartIndex, EndIndex, VNI));
+          for (auto &SR : LI.subranges())
+            SR.addSegment(LiveInterval::Segment(StartIndex, EndIndex, VNI));
         }
       }
     }
@@ -1294,8 +1296,18 @@ MachineBasicBlock *MachineBasicBlock::SplitCriticalEdge(
         VNInfo *VNI = LI.getVNInfoAt(PrevIndex);
         assert(VNI && "LiveInterval should have VNInfo where it is live.");
         LI.addSegment(LiveInterval::Segment(StartIndex, EndIndex, VNI));
+        // Update subranges with live values
+        for (auto &SR : LI.subranges()) {
+          VNInfo *VNI = SR.getVNInfoAt(PrevIndex);
+          if (VNI)
+            SR.addSegment(LiveInterval::Segment(StartIndex, EndIndex, VNI));
+        }
       } else if (!isLiveOut && !isLastMBB) {
         LI.removeSegment(StartIndex, EndIndex);
+        for (auto &SR : LI.subranges()) {
+          if (SR.overlaps(StartIndex, EndIndex))
+            SR.removeSegment(StartIndex, EndIndex);
+        }
       }
     }
 
diff --git a/llvm/lib/CodeGen/PHIElimination.cpp b/llvm/lib/CodeGen/PHIElimination.cpp
index 10d8378ce58d1cf..df4dae11f4d1faf 100644
--- a/llvm/lib/CodeGen/PHIElimination.cpp
+++ b/llvm/lib/CodeGen/PHIElimination.cpp
@@ -136,6 +136,7 @@ INITIALIZE_PASS_END(PHIElimination, DEBUG_TYPE,
 
 void PHIElimination::getAnalysisUsage(AnalysisUsage &AU) const {
   AU.addUsedIfAvailable<LiveVariables>();
+  AU.addUsedIfAvailable<LiveIntervals>();
   AU.addPreserved<LiveVariables>();
   AU.addPreserved<SlotIndexes>();
   AU.addPreserved<LiveIntervals>();
@@ -389,7 +390,7 @@ void PHIElimination::LowerPHINode(MachineBasicBlock &MBB,
     if (IncomingReg) {
       // Add the region from the beginning of MBB to the copy instruction to
       // IncomingReg's live interval.
-      LiveInterval &IncomingLI = LIS->createEmptyInterval(IncomingReg);
+      LiveInterval &IncomingLI = LIS->getOrCreateEmptyInterval(IncomingReg);
       VNInfo *IncomingVNI = IncomingLI.getVNInfoAt(MBBStartIndex);
       if (!IncomingVNI)
         IncomingVNI = IncomingLI.getNextValue(MBBStartIndex,
@@ -400,24 +401,49 @@ void PHIElimination::LowerPHINode(MachineBasicBlock &MBB,
     }
 
     LiveInterval &DestLI = LIS->getInterval(DestReg);
-    assert(!DestLI.empty() && "PHIs should have nonempty LiveIntervals.");
-    if (DestLI.endIndex().isDead()) {
-      // A dead PHI's live range begins and ends at the start of the MBB, but
-      // the lowered copy, which will still be dead, needs to begin and end at
-      // the copy instruction.
-      VNInfo *OrigDestVNI = DestLI.getVNInfoAt(MBBStartIndex);
-      assert(OrigDestVNI && "PHI destination should be live at block entry.");
-      DestLI.removeSegment(MBBStartIndex, MBBStartIndex.getDeadSlot());
-      DestLI.createDeadDef(DestCopyIndex.getRegSlot(),
-                           LIS->getVNInfoAllocator());
-      DestLI.removeValNo(OrigDestVNI);
-    } else {
-      // Otherwise, remove the region from the beginning of MBB to the copy
-      // instruction from DestReg's live interval.
-      DestLI.removeSegment(MBBStartIndex, DestCopyIndex.getRegSlot());
-      VNInfo *DestVNI = DestLI.getVNInfoAt(DestCopyIndex.getRegSlot());
+    assert(!DestLI.empty() && "PHIs should have non-empty LiveIntervals.");
+
+    SlotIndex NewStart = DestCopyIndex.getRegSlot();
+
+    SmallVector<LiveRange*> ToUpdate;
+    ToUpdate.push_back(&DestLI);
+    for (auto &SR : DestLI.subranges())
+      ToUpdate.push_back(&SR);
+
+    for (auto LR : ToUpdate) {
+      auto DestSegment = LR->find(MBBStartIndex);
+      assert(DestSegment != LR->end() && "PHI destination must be live in block");
+
+      if (LR->endIndex().isDead()) {
+        // A dead PHI's live range begins and ends at the start of the MBB, but
+        // the lowered copy, which will still be dead, needs to begin and end at
+        // the copy instruction.
+        VNInfo *OrigDestVNI = LR->getVNInfoAt(DestSegment->start);
+        assert(OrigDestVNI && "PHI destination should be live at block entry.");
+        LR->removeSegment(DestSegment->start, DestSegment->start.getDeadSlot());
+        LR->createDeadDef(NewStart, LIS->getVNInfoAllocator());
+        LR->removeValNo(OrigDestVNI);
+        continue;
+      }
+
+      if (DestSegment->start > NewStart) {
+        // With a single PHI removed from block the index of the copy may be
+        // lower than the original PHI. Extend live range backward to cover
+        // the copy.
+        VNInfo *VNI = LR->getVNInfoAt(DestSegment->start);
+        assert(VNI && "value should be defined for known segment");
+        LR->addSegment(LiveInterval::Segment(
+            NewStart, DestSegment->start, VNI));
+      } else if (DestSegment->start < NewStart) {
+        // Otherwise, remove the region from the beginning of MBB to the copy
+        // instruction from DestReg's live interval.
+        assert(DestSegment->start >= MBBStartIndex);
+        assert(DestSegment->end >= DestCopyIndex.getRegSlot());
+        LR->removeSegment(DestSegment->start, NewStart);
+      }
+      VNInfo *DestVNI = LR->getVNInfoAt(NewStart);
       assert(DestVNI && "PHI destination should be live at its definition.");
-      DestVNI->def = DestCopyIndex.getRegSlot();
+      DestVNI->def = NewStart;
     }
   }
 
@@ -612,6 +638,10 @@ void PHIElimination::LowerPHINode(MachineBasicBlock &MBB,
           SlotIndex LastUseIndex = LIS->getInstructionIndex(*KillInst);
           SrcLI.removeSegment(LastUseIndex.getRegSlot(),
                               LIS->getMBBEndIdx(&opBlock));
+          for (auto &SR : SrcLI.subranges()) {
+            SR.removeSegment(LastUseIndex.getRegSlot(),
+                                LIS->getMBBEndIdx(&opBlock));
+          }
         }
       }
     }
diff --git a/llvm/test/CodeGen/AMDGPU/split-mbb-lis-subrange.mir b/llvm/test/CodeGen/AMDGPU/split-mbb-lis-subrange.mir
index 9ee99a27dc84ff8..d5f38d0451a8c59 100644
--- a/llvm/test/CodeGen/AMDGPU/split-mbb-lis-subrange.mir
+++ b/llvm/test/CodeGen/AMDGPU/split-mbb-lis-subrange.mir
@@ -1,7 +1,7 @@
 # NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 2
-# RUN: llc -march=amdgcn -mcpu=gfx1100 -verify-machineinstrs -run-pass liveintervals -o - %s | FileCheck -check-prefixes=GCN %s
+# RUN: llc -march=amdgcn -mcpu=gfx1100 -verify-machineinstrs -run-pass liveintervals,phi-node-elimination -o - %s | FileCheck -check-prefixes=GCN %s
 
-# This test simply checks that liveintervals pass verification.
+# This checks liveintervals pass verification and phi-node-elimination correctly preserves them.
 
 ---
 name: split_critical_edge_subranges
@@ -9,7 +9,7 @@ tracksRegLiveness: true
 body:             |
   ; GCN-LABEL: name: split_critical_edge_subranges
   ; GCN: bb.0:
-  ; GCN-NEXT:   successors: %bb.3(0x40000000), %bb.1(0x40000000)
+  ; GCN-NEXT:   successors: %bb.5(0x40000000), %bb.1(0x40000000)
   ; GCN-NEXT: {{  $}}
   ; GCN-NEXT:   %coord:vreg_64 = IMPLICIT_DEF
   ; GCN-NEXT:   %desc:sgpr_256 = IMPLICIT_DEF
@@ -20,14 +20,22 @@ body:             |
   ; GCN-NEXT:   %s0a:vgpr_32 = COPY %load.sub0
   ; GCN-NEXT:   %s0b:vgpr_32 = COPY %load.sub1
   ; GCN-NEXT:   S_CMP_EQ_U32 %c0, %c1, implicit-def $scc
-  ; GCN-NEXT:   S_CBRANCH_SCC1 %bb.3, implicit $scc
-  ; GCN-NEXT:   S_BRANCH %bb.1
+  ; GCN-NEXT:   S_CBRANCH_SCC0 %bb.1, implicit $scc
+  ; GCN-NEXT: {{  $}}
+  ; GCN-NEXT: bb.5:
+  ; GCN-NEXT:   successors: %bb.3(0x80000000)
+  ; GCN-NEXT: {{  $}}
+  ; GCN-NEXT:   [[COPY:%[0-9]+]]:vgpr_32 = COPY %s0a
+  ; GCN-NEXT:   [[COPY1:%[0-9]+]]:vgpr_32 = COPY %s0b
+  ; GCN-NEXT:   S_BRANCH %bb.3
   ; GCN-NEXT: {{  $}}
   ; GCN-NEXT: bb.1:
   ; GCN-NEXT:   successors: %bb.3(0x80000000)
   ; GCN-NEXT: {{  $}}
   ; GCN-NEXT:   %s0c:vgpr_32 = V_ADD_F32_e64 0, %s0a, 0, %const, 0, 0, implicit $mode, implicit $exec
   ; GCN-NEXT:   %s0d:vgpr_32 = V_ADD_F32_e64 0, %s0b, 0, %const, 0, 0, implicit $mode, implicit $exec
+  ; GCN-NEXT:   [[COPY2:%[0-9]+]]:vgpr_32 = COPY %s0c
+  ; GCN-NEXT:   [[COPY3:%[0-9]+]]:vgpr_32 = COPY %s0d
   ; GCN-NEXT:   S_BRANCH %bb.3
   ; GCN-NEXT: {{  $}}
   ; GCN-NEXT: bb.2:
@@ -37,8 +45,8 @@ body:             |
   ; GCN-NEXT: bb.3:
   ; GCN-NEXT:   successors: %bb.4(0x80000000)
   ; GCN-NEXT: {{  $}}
-  ; GCN-NEXT:   %phi0:vgpr_32 = PHI %s0a, %bb.0, %s0c, %bb.1
-  ; GCN-NEXT:   %phi1:vgpr_32 = PHI %s0b, %bb.0, %s0d, %bb.1
+  ; GCN-NEXT:   %phi1:vgpr_32 = COPY [[COPY3]]
+  ; GCN-NEXT:   %phi0:vgpr_32 = COPY [[COPY2]]
   ; GCN-NEXT:   S_BRANCH %bb.4
   ; GCN-NEXT: {{  $}}
   ; GCN-NEXT: bb.4:

perlfu · 2023-10-18T07:58:33Z

Was differential revision: https://reviews.llvm.org/D158144

I think we were already happy with this after splitting out SplitCriticalEdge fix, but it has been a little while.

github-actions · 2023-10-18T08:08:40Z

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

llvm/lib/CodeGen/PHIElimination.cpp

llvm/lib/CodeGen/MachineBasicBlock.cpp

MatzeB

Looks mostly fine
I have some questions on one if condition below.
Should probably mention in the summary that this adds subrange liveness tracking for critical edge splitting and phi elimination.

llvm/lib/CodeGen/LiveInterval.cpp

MatzeB · 2023-10-24T00:05:14Z

llvm/lib/CodeGen/PHIElimination.cpp

+      if (DestSegment->start > NewStart) {
+        // With a single PHI removed from block the index of the copy may be
+        // lower than the original PHI. Extend live range backward to cover
+        // the copy.


I find this comment highly confusing, and have a hard time seeing when this condition would ever be true... There was a PHI definition DestReg here, no? That should mean that every segment here should have been defined at MBBStartIndex (where PHI definitions take effect), no? (Except maybe if a subrange was undefined, but then we would have already entered the isDead() case above...)

I have moved and reworded this comment for clarity.

There are two details here that matter to understanding this:

LiveIntervals of PHIs destinations start at the PHI slot index, not the block start.

Destination copies are not inserted in the order of the PHIs they replace.

Taking the example of the attached test.
Original PHIs look like this:

320B %phi0:vgpr_32 = PHI %s0a:vgpr_32, %bb.0, %s0c:vgpr_32, %bb.1 336B %phi1:vgpr_32 = PHI %s0b:vgpr_32, %bb.0, %s0d:vgpr_32, %bb.1

With intervals as follows:

%6 [112r,208r:0) 0@112r weight:0.000000e+00 -- s0a %7 [128r,224r:0) 0@128r weight:0.000000e+00 -- s0b %8 [208r,256B:0) 0@208r weight:0.000000e+00 -- s0c %9 [224r,256B:0) 0@224r weight:0.000000e+00 -- s0d %10 [320r,384r:0) 0@320r weight:0.000000e+00 -- phi0 %11 [336r,384r:0) 0@336r weight:0.000000e+00 -- phi1

phi0 is lowered, giving the following IR:

336B %phi1:vgpr_32 = PHI %s0b:vgpr_32, %bb.0, %s0d:vgpr_32, %bb.1 344B %phi0:vgpr_32 = COPY %12:vgpr_32

phi1 is lowered, giving:

312B %phi1:vgpr_32 = COPY %13:vgpr_32 344B %phi0:vgpr_32 = COPY %12:vgpr_32

Hence COPY for phi0 has index after original PHI, but phi1 has index before original PHI.

MatzeB · 2023-10-24T00:05:55Z

llvm/lib/CodeGen/PHIElimination.cpp

+      } else if (DestSegment->start < NewStart) {
+        // Otherwise, remove the region from the beginning of MBB to the copy
+        // instruction from DestReg's live interval.
+        assert(DestSegment->start >= MBBStartIndex);


as above this should be ==, no?

Add handling for subrange updates in LiveInterval preservation. Differential Revision: https://reviews.llvm.org/D158144

- Modify LiveRange::removeSegment so it handles intervals with no Segment - Update code to remove overlaps() checks - Remove addUsedIfAvailable of LiveIntervals

- Remove duplicate comment. - Update comments about order of PHI nodes versus COPYs.

perlfu · 2023-11-03T07:55:16Z

Ping

perlfu · 2023-11-06T06:11:37Z

I will merge this on the 9th of November if there is no further feedback.

jayfoad

Looks good but I will defer to @MatzeB.

llvm/include/llvm/CodeGen/LiveIntervals.h

llvm/lib/CodeGen/PHIElimination.cpp

Add subrange tracking and handling for LiveIntervals during PHI elimination. This requires extending MachineBasicBlock::SplitCriticalEdge to also update subrange intervals.

perlfu requested review from jayfoad and arsenm October 18, 2023 07:57

llvmbot added backend:AMDGPU llvm:regalloc labels Oct 18, 2023

perlfu force-pushed the phi-elimination-lis-subrange branch from f19a5ab to 001af0f Compare October 18, 2023 10:31

jayfoad requested review from MatzeB, fhahn and qcolombet October 18, 2023 12:21

jayfoad reviewed Oct 18, 2023

View reviewed changes

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

jayfoad reviewed Oct 18, 2023

View reviewed changes

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

perlfu mentioned this pull request Oct 19, 2023

[MachineBasicBlock] Fix SlotIndexUpdater for insertion order #69424

Merged

MatzeB reviewed Oct 24, 2023

View reviewed changes

perlfu added 3 commits October 30, 2023 15:23

[PHIElimination] Handle subranges in LiveInterval updates

c3f698e

Add handling for subrange updates in LiveInterval preservation. Differential Revision: https://reviews.llvm.org/D158144

Address reviewer feedback:

4f26b46

- Modify LiveRange::removeSegment so it handles intervals with no Segment - Update code to remove overlaps() checks - Remove addUsedIfAvailable of LiveIntervals

Address reviewer feedback:

e2ff448

- Remove duplicate comment. - Update comments about order of PHI nodes versus COPYs.

perlfu force-pushed the phi-elimination-lis-subrange branch from 8aa7ec6 to e2ff448 Compare October 31, 2023 08:14

jayfoad reviewed Nov 7, 2023

View reviewed changes

llvm/include/llvm/CodeGen/LiveIntervals.h Show resolved Hide resolved

Add doxygen comment for getOrCreateEmptyInterval.

6c822ea

arsenm reviewed Nov 8, 2023

View reviewed changes

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

Simplify initialisation of ToUpdate vector.

a4933c7

arsenm approved these changes Nov 9, 2023

View reviewed changes

perlfu merged commit 52b247b into llvm:main Nov 13, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PHIElimination] Handle subranges in LiveInterval updates #69429

[PHIElimination] Handle subranges in LiveInterval updates #69429

perlfu commented Oct 18, 2023 •

edited

llvmbot commented Oct 18, 2023 •

edited

perlfu commented Oct 18, 2023

github-actions bot commented Oct 18, 2023 •

edited

MatzeB left a comment

MatzeB Oct 24, 2023

perlfu Oct 31, 2023

MatzeB Oct 24, 2023

perlfu commented Nov 3, 2023

perlfu commented Nov 6, 2023

jayfoad left a comment

[PHIElimination] Handle subranges in LiveInterval updates #69429

[PHIElimination] Handle subranges in LiveInterval updates #69429

Conversation

perlfu commented Oct 18, 2023 • edited

llvmbot commented Oct 18, 2023 • edited

perlfu commented Oct 18, 2023

github-actions bot commented Oct 18, 2023 • edited

MatzeB left a comment

Choose a reason for hiding this comment

MatzeB Oct 24, 2023

Choose a reason for hiding this comment

perlfu Oct 31, 2023

Choose a reason for hiding this comment

MatzeB Oct 24, 2023

Choose a reason for hiding this comment

perlfu commented Nov 3, 2023

perlfu commented Nov 6, 2023

jayfoad left a comment

Choose a reason for hiding this comment

perlfu commented Oct 18, 2023 •

edited

llvmbot commented Oct 18, 2023 •

edited

github-actions bot commented Oct 18, 2023 •

edited