AMDGPU: Update live intervals in convertToThreeAddress #104610

arsenm · 2024-08-16T15:54:44Z

Fixes #98741

arsenm · 2024-08-16T15:55:00Z

AMDGPU: Update live intervals in convertToThreeAddress #104610 👈
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @arsenm and the rest of your teammates on Graphite

llvmbot · 2024-08-16T15:55:12Z

@llvm/pr-subscribers-backend-amdgpu

Author: Matt Arsenault (arsenm)

Changes

Fixes #98741

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

2 Files Affected:

(modified) llvm/lib/Target/AMDGPU/SIInstrInfo.cpp (+30-7)
(modified) llvm/test/CodeGen/AMDGPU/gfx10-twoaddr-fma.mir (+102-11)

diff --git a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
index 9147242046ceda..0e3a12d49137cc 100644
--- a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
+++ b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
@@ -3947,14 +3947,32 @@ MachineInstr *SIInstrInfo::convertToThreeAddress(MachineInstr &MI,
       (ST.getConstantBusLimit(Opc) > 1 || !Src0->isReg() ||
        !RI.isSGPRReg(MBB.getParent()->getRegInfo(), Src0->getReg()))) {
     MachineInstr *DefMI;
-    const auto killDef = [&]() -> void {
+    const auto killDef = [&](SlotIndex NewIdx) -> void {
       const MachineRegisterInfo &MRI = MBB.getParent()->getRegInfo();
       // The only user is the instruction which will be killed.
       Register DefReg = DefMI->getOperand(0).getReg();
+
+      if (LIS) {
+        LiveInterval &DefLI = LIS->getInterval(DefReg);
+        LiveRange::Segment *OldSeg = DefLI.getSegmentContaining(NewIdx);
+
+        if (OldSeg->end == NewIdx.getRegSlot()) {
+          DefLI.removeSegment(OldSeg->start, NewIdx.getRegSlot(), true);
+
+          for (auto &SR : DefLI.subranges()) {
+            LiveRange::Segment *OldSegSR = SR.getSegmentContaining(NewIdx);
+            SR.removeSegment(OldSegSR->start, NewIdx.getRegSlot(), true);
+          }
+
+          DefLI.removeEmptySubRanges();
+        }
+      }
+
       if (!MRI.hasOneNonDBGUse(DefReg))
         return;
       // We cannot just remove the DefMI here, calling pass will crash.
       DefMI->setDesc(get(AMDGPU::IMPLICIT_DEF));
+      DefMI->getOperand(0).setIsDead(true);
       for (unsigned I = DefMI->getNumOperands() - 1; I != 0; --I)
         DefMI->removeOperand(I);
       if (LV)
@@ -3976,9 +3994,10 @@ MachineInstr *SIInstrInfo::convertToThreeAddress(MachineInstr &MI,
                   .addImm(Imm)
                   .setMIFlags(MI.getFlags());
         updateLiveVariables(LV, MI, *MIB);
+        SlotIndex NewIdx;
         if (LIS)
-          LIS->ReplaceMachineInstrInMaps(MI, *MIB);
-        killDef();
+          NewIdx = LIS->ReplaceMachineInstrInMaps(MI, *MIB);
+        killDef(NewIdx);
         return MIB;
       }
     }
@@ -3996,9 +4015,11 @@ MachineInstr *SIInstrInfo::convertToThreeAddress(MachineInstr &MI,
                   .add(*Src2)
                   .setMIFlags(MI.getFlags());
         updateLiveVariables(LV, MI, *MIB);
+
+        SlotIndex NewIdx;
         if (LIS)
-          LIS->ReplaceMachineInstrInMaps(MI, *MIB);
-        killDef();
+          NewIdx = LIS->ReplaceMachineInstrInMaps(MI, *MIB);
+        killDef(NewIdx);
         return MIB;
       }
     }
@@ -4018,10 +4039,12 @@ MachineInstr *SIInstrInfo::convertToThreeAddress(MachineInstr &MI,
                   .add(*Src2)
                   .setMIFlags(MI.getFlags());
         updateLiveVariables(LV, MI, *MIB);
+
+        SlotIndex NewIdx;
         if (LIS)
-          LIS->ReplaceMachineInstrInMaps(MI, *MIB);
+          NewIdx = LIS->ReplaceMachineInstrInMaps(MI, *MIB);
         if (DefMI)
-          killDef();
+          killDef(NewIdx);
         return MIB;
       }
     }
diff --git a/llvm/test/CodeGen/AMDGPU/gfx10-twoaddr-fma.mir b/llvm/test/CodeGen/AMDGPU/gfx10-twoaddr-fma.mir
index 0f20b8a2f1e29f..bde2ae13c4beab 100644
--- a/llvm/test/CodeGen/AMDGPU/gfx10-twoaddr-fma.mir
+++ b/llvm/test/CodeGen/AMDGPU/gfx10-twoaddr-fma.mir
@@ -1,29 +1,120 @@
 # RUN: llc -mtriple=amdgcn -mcpu=gfx1010 %s -run-pass twoaddressinstruction -verify-machineinstrs -o - | FileCheck --check-prefixes=GFX10 %s
 # RUN: llc -mtriple=amdgcn -mcpu=gfx1010 %s --passes=two-address-instruction -o - | FileCheck --check-prefixes=GFX10 %s
+# RUN: llc -march=amdgcn -mcpu=gfx1010 -run-pass liveintervals -run-pass twoaddressinstruction -verify-machineinstrs %s -o -
 
 # GFX10-LABEL: name: test_fmamk_reg_imm_f16
-# GFX10: %2:vgpr_32 = IMPLICIT_DEF
+# GFX10: dead %2:vgpr_32 = IMPLICIT_DEF
 # GFX10-NOT: V_MOV_B32
 # GFX10: V_FMAMK_F16 killed %0.sub0, 1078523331, killed %1, implicit $mode, implicit $exec
 ---
 name:            test_fmamk_reg_imm_f16
-registers:
-  - { id: 0, class: vreg_64 }
-  - { id: 1, class: vgpr_32 }
-  - { id: 2, class: vgpr_32 }
-  - { id: 3, class: vgpr_32 }
 body:             |
   bb.0:
 
-    %0 = IMPLICIT_DEF
-    %1 = COPY %0.sub1
-    %2 = V_MOV_B32_e32 1078523331, implicit $exec
-    %3 = V_FMAC_F16_e32 killed %0.sub0, %2, killed %1, implicit $mode, implicit $exec
+    %0:vreg_64 = IMPLICIT_DEF
+    %1:vgpr_32 = COPY %0.sub1
+    %2:vgpr_32 = V_MOV_B32_e32 1078523331, implicit $exec
+    %3:vgpr_32 = V_FMAC_F16_e32 killed %0.sub0, %2, killed %1, implicit $mode, implicit $exec
+
+...
+
+# GFX10-LABEL: name: test_fmamk_reg_imm_f16__imm_is_subreg
+# GFX10: %0:vreg_64 = IMPLICIT_DEF
+# GFX10: %1:vgpr_32 = COPY %0.sub1
+# GFX10: dead undef %2.sub0:vreg_64 = IMPLICIT_DEF
+# GFX10: %3:vgpr_32 = V_FMAMK_F16 killed %0.sub0, 1078523331, killed %1, implicit $mode, implicit $exec
+---
+name:            test_fmamk_reg_imm_f16__imm_is_subreg
+body:             |
+  bb.0:
+
+    %0:vreg_64 = IMPLICIT_DEF
+    %1:vgpr_32 = COPY %0.sub1
+    undef %2.sub0:vreg_64 = V_MOV_B32_e32 1078523331, implicit $exec
+    %3:vgpr_32 = V_FMAC_F16_e32 killed %0.sub0, %2.sub0, killed %1, implicit $mode, implicit $exec
+
+...
+
+# GFX10-LABEL: name: test_fmamk_reg_imm_f16__imm_is_subreg_fully_defined
+# GFX10: %0:vreg_64 = IMPLICIT_DEF
+# GFX10: %1:vgpr_32 = COPY %0.sub1
+# GFX10: undef %2.sub1:vreg_64 = V_MOV_B32_e32 9999, implicit $exec
+# GFX10: %2.sub0:vreg_64 = V_MOV_B32_e32 1078523331, implicit $exec
+# GFX10: %3:vgpr_32 = V_FMA_F16_gfx9_e64 0, killed %0.sub0, 0, %2.sub0, 0, killed %1, 0, 0, 0, implicit $mode, implicit $e
+---
+name:            test_fmamk_reg_imm_f16__imm_is_subreg_fully_defined
+body:             |
+  bb.0:
+
+    %0:vreg_64 = IMPLICIT_DEF
+    %1:vgpr_32 = COPY %0.sub1
+    undef %2.sub1 = V_MOV_B32_e32 9999, implicit $exec
+    %2.sub0:vreg_64 = V_MOV_B32_e32 1078523331, implicit $exec
+    %3:vgpr_32 = V_FMAC_F16_e32 killed %0.sub0, %2.sub0, killed %1, implicit $mode, implicit $exec
+
+...
+
+# GFX10-LABEL: name: test_fmamk_reg_imm_f16__use_imm_before_mac
+# GFX10: %0:vreg_64 = IMPLICIT_DEF
+# GFX10: %1:vgpr_32 = COPY %0.sub1
+# GFX10: %2:vgpr_32 = V_MOV_B32_e32 1078523331, implicit $exec
+# GFX10: S_NOP 0, implicit %2
+# GFX10: %3:vgpr_32 = V_FMAMK_F16 killed %0.sub0, 1078523331, killed %1, implicit $mode, implicit $exec
+---
+name:            test_fmamk_reg_imm_f16__use_imm_before_mac
+body:             |
+  bb.0:
+
+    %0:vreg_64 = IMPLICIT_DEF
+    %1:vgpr_32 = COPY %0.sub1
+    %2:vgpr_32 = V_MOV_B32_e32 1078523331, implicit $exec
+    S_NOP 0, implicit %2
+    %3:vgpr_32 = V_FMAC_F16_e32 killed %0.sub0, %2, killed %1, implicit $mode, implicit $exec
+
+...
+
+# GFX10-LABEL: name: test_fmamk_reg_imm_f16__use_imm_after_mac
+# GFX10: %0:vreg_64 = IMPLICIT_DEF
+# GFX10: %1:vgpr_32 = COPY %0.sub1
+# GFX10: %2:vgpr_32 = V_MOV_B32_e32 1078523331, implicit $exec
+# GFX10: %3:vgpr_32 = V_FMAMK_F16 killed %0.sub0, 1078523331, killed %1, implicit $mode, implicit $exec
+---
+name:            test_fmamk_reg_imm_f16__use_imm_after_mac
+body:             |
+  bb.0:
+
+    %0:vreg_64 = IMPLICIT_DEF
+    %1:vgpr_32 = COPY %0.sub1
+    %2:vgpr_32 = V_MOV_B32_e32 1078523331, implicit $exec
+    %3:vgpr_32 = V_FMAC_F16_e32 killed %0.sub0, %2, killed %1, implicit $mode, implicit $exec
+    S_NOP 0, implicit %2
+
+...
+
+# GFX10-LABEL: name: test_fmamk_reg_imm_f16__use_imm_before_after_mac
+# GFX10: %0:vreg_64 = IMPLICIT_DEF
+# GFX10: %1:vgpr_32 = COPY %0.sub1
+# GFX10: %2:vgpr_32 = V_MOV_B32_e32 1078523331, implicit $exec
+# GFX10: S_NOP 0, implicit %2
+# GFX10: %3:vgpr_32 = V_FMAMK_F16 killed %0.sub0, 1078523331, killed %1, implicit $mode, implicit $exec
+# GFX10: S_NOP 0, implicit %2
+
+---
+name:            test_fmamk_reg_imm_f16__use_imm_before_after_mac
+body:             |
+  bb.0:
+
+    %0:vreg_64 = IMPLICIT_DEF
+    %1:vgpr_32 = COPY %0.sub1
+    %2:vgpr_32 = V_MOV_B32_e32 1078523331, implicit $exec
+    S_NOP 0, implicit %2
+    %3:vgpr_32 = V_FMAC_F16_e32 killed %0.sub0, %2, killed %1, implicit $mode, implicit $exec
+    S_NOP 0, implicit %2
 
 ...
 
 # GFX10-LABEL: name: test_fmamk_imm_reg_f16
-# GFX10: %2:vgpr_32 = IMPLICIT_DEF
+# GFX10: dead %2:vgpr_32 = IMPLICIT_DEF
 # GFX10-NOT: V_MOV_B32
 # GFX10: V_FMAMK_F16 killed %0.sub0, 1078523331, killed %1, implicit $mode, implicit $exec
 ---

llvm/test/CodeGen/AMDGPU/gfx10-twoaddr-fma.mir

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

perlfu · 2024-08-19T09:08:56Z

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

      if (!MRI.hasOneNonDBGUse(DefReg))
        return;
      // We cannot just remove the DefMI here, calling pass will crash.
      DefMI->setDesc(get(AMDGPU::IMPLICIT_DEF));
+      DefMI->getOperand(0).setIsDead(true);
      for (unsigned I = DefMI->getNumOperands() - 1; I != 0; --I)
        DefMI->removeOperand(I);


Is it possible that we actually need to update intervals for any of these operands being removed as well?

The operand this should be removing is exec, so no. In principle there could be other implicit use operands, but they add a lot of complexity for no reason so I would hope movs with implicit operands would have been rejected as fold candidates

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

perlfu

LGTM with the one comment about variable naming

perlfu · 2024-09-02T05:31:06Z

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

@@ -3947,14 +3947,34 @@ MachineInstr *SIInstrInfo::convertToThreeAddress(MachineInstr &MI,
      (ST.getConstantBusLimit(Opc) > 1 || !Src0->isReg() ||
       !RI.isSGPRReg(MBB.getParent()->getRegInfo(), Src0->getReg()))) {
    MachineInstr *DefMI;
-    const auto killDef = [&]() -> void {
+    const auto killDef = [&](SlotIndex OldDefIdx) -> void {


This variable should be called OldUseIdx.

perlfu · 2024-09-06T05:48:10Z

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

@@ -3934,18 +3934,38 @@ MachineInstr *SIInstrInfo::convertToThreeAddress(MachineInstr &MI,
      (ST.getConstantBusLimit(Opc) > 1 || !Src0->isReg() ||
       !RI.isSGPRReg(MBB.getParent()->getRegInfo(), Src0->getReg()))) {
    MachineInstr *DefMI;
-    const auto killDef = [&]() -> void {
-      const MachineRegisterInfo &MRI = MBB.getParent()->getRegInfo();
+    const auto killDef = [&](SlotIndex OldUseIdx) -> void {


OldUseIdx is no longer required in this version.

perlfu · 2024-09-06T05:48:38Z

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

+      if (LIS) {
+        LiveInterval &DefLI = LIS->getInterval(DefReg);
+
+        // We cannot delete the original instruction here, so hack out the use


Can you add a test that demonstrates this?

This is already covered by several tests (the problematic cases were any of the ones with other uses)

Fixes #98741

arsenm · 2024-09-06T14:14:44Z

Merge activity

Sep 6, 10:14 AM EDT: @arsenm started a stack merge that includes this pull request via Graphite.
Sep 6, 10:18 AM EDT: @arsenm merged this pull request with Graphite.

arsenm added the backend:AMDGPU label Aug 16, 2024 — with Graphite App

arsenm requested review from cdevadas, jayfoad, paperchalice, perlfu, piotrAMD, rampitec and Sisyph August 16, 2024 15:55

arsenm marked this pull request as ready for review August 16, 2024 15:56

jayfoad reviewed Aug 16, 2024

View reviewed changes

llvm/test/CodeGen/AMDGPU/gfx10-twoaddr-fma.mir Outdated Show resolved Hide resolved

arsenm force-pushed the users/arsenm/issue98741-twoaddr-insts-live-interval-update branch 2 times, most recently from db8d4ae to 807378a Compare August 16, 2024 16:13

perlfu reviewed Aug 19, 2024

View reviewed changes

arsenm force-pushed the users/arsenm/issue98741-twoaddr-insts-live-interval-update branch from 807378a to 5b1d897 Compare August 19, 2024 13:28

perlfu approved these changes Sep 2, 2024

View reviewed changes

arsenm force-pushed the users/arsenm/issue98741-twoaddr-insts-live-interval-update branch from 5b1d897 to 5ae0d57 Compare September 4, 2024 14:05

arsenm requested review from perlfu and jayfoad September 6, 2024 05:36

perlfu reviewed Sep 6, 2024

View reviewed changes

arsenm added 8 commits September 6, 2024 10:34

AMDGPU: Update live intervals in convertToThreeAddress

c71c5dd

Fixes #98741

Comments

15573e4

Rename variable

a332148

Rename variable

270f780

Remove less

1fc765d

Add tracksRegLiveness to every function

7d7a5e6

Hackily use shrinkToUses

61b61f2

Remove unused index argument

7a56477

arsenm force-pushed the users/arsenm/issue98741-twoaddr-insts-live-interval-update branch from 5ae0d57 to 7a56477 Compare September 6, 2024 07:17

arsenm merged commit a9daad8 into main Sep 6, 2024
8 checks passed

arsenm deleted the users/arsenm/issue98741-twoaddr-insts-live-interval-update branch September 6, 2024 14:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AMDGPU: Update live intervals in convertToThreeAddress #104610

AMDGPU: Update live intervals in convertToThreeAddress #104610

arsenm commented Aug 16, 2024

arsenm commented Aug 16, 2024

llvmbot commented Aug 16, 2024

perlfu Aug 19, 2024

arsenm Aug 19, 2024

perlfu left a comment

perlfu Sep 2, 2024

perlfu Sep 6, 2024

perlfu Sep 6, 2024

arsenm Sep 6, 2024

arsenm commented Sep 6, 2024 •

edited

Loading

AMDGPU: Update live intervals in convertToThreeAddress #104610

AMDGPU: Update live intervals in convertToThreeAddress #104610

Conversation

arsenm commented Aug 16, 2024

arsenm commented Aug 16, 2024

llvmbot commented Aug 16, 2024

perlfu Aug 19, 2024

Choose a reason for hiding this comment

arsenm Aug 19, 2024

Choose a reason for hiding this comment

perlfu left a comment

Choose a reason for hiding this comment

perlfu Sep 2, 2024

Choose a reason for hiding this comment

perlfu Sep 6, 2024

Choose a reason for hiding this comment

perlfu Sep 6, 2024

Choose a reason for hiding this comment

arsenm Sep 6, 2024

Choose a reason for hiding this comment

arsenm commented Sep 6, 2024 • edited Loading

Merge activity

arsenm commented Sep 6, 2024 •

edited

Loading