Skip to content
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.

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

[X86] Skip unused VRegs traverse #78229

Merged
merged 2 commits into from
Jan 26, 2024
Merged

[X86] Skip unused VRegs traverse #78229

merged 2 commits into from
Jan 26, 2024

Conversation

e-kud
Copy link
Contributor

@e-kud e-kud commented Jan 16, 2024

Almost all loops with getNumVirtRegs skip unused registers by reg_nodbg_empty or empty live interval. Except for these two cases that are revealed by GlobalISel since it can skip RegClass assignment for unused registers.

Closes #64452, closes #71926

GlobalISel may leave dead VRegs without a register class. It shouldn't
be a problem if instructions or VRegs are traversed correctly.

Closes llvm#64452 llvm#71926
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 16, 2024

@llvm/pr-subscribers-backend-x86

Author: Evgenii Kudriashov (e-kud)

Changes

Almost all loops with getNumVirtRegs skip unused registers by reg_nodbg_empty or empty live interval. Except for these two cases that are revealed by GlobalISel since it can skip RegClass assignment for unused registers.

Closes #64452 #71926


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

4 Files Affected:

  • (modified) llvm/lib/Target/X86/X86DomainReassignment.cpp (+4)
  • (modified) llvm/lib/Target/X86/X86FastPreTileConfig.cpp (+2-1)
  • (modified) llvm/test/CodeGen/X86/AMX/amx-fastpreconfig.mir (+45)
  • (modified) llvm/test/CodeGen/X86/domain-reassignment.mir (+64)
diff --git a/llvm/lib/Target/X86/X86DomainReassignment.cpp b/llvm/lib/Target/X86/X86DomainReassignment.cpp
index 20dbaf797e3272..53c0486c8697fd 100644
--- a/llvm/lib/Target/X86/X86DomainReassignment.cpp
+++ b/llvm/lib/Target/X86/X86DomainReassignment.cpp
@@ -754,6 +754,10 @@ bool X86DomainReassignment::runOnMachineFunction(MachineFunction &MF) {
   for (unsigned Idx = 0; Idx < MRI->getNumVirtRegs(); ++Idx) {
     Register Reg = Register::index2VirtReg(Idx);
 
+    // Skip unused VRegs.
+    if (MRI->reg_nodbg_empty(Reg))
+      continue;
+
     // GPR only current source domain supported.
     if (!isGPR(MRI->getRegClass(Reg)))
       continue;
diff --git a/llvm/lib/Target/X86/X86FastPreTileConfig.cpp b/llvm/lib/Target/X86/X86FastPreTileConfig.cpp
index ea942445a18193..7708089074c0a3 100644
--- a/llvm/lib/Target/X86/X86FastPreTileConfig.cpp
+++ b/llvm/lib/Target/X86/X86FastPreTileConfig.cpp
@@ -667,7 +667,8 @@ bool X86FastPreTileConfig::runOnMachineFunction(MachineFunction &MFunc) {
   bool HasVirtTileReg = false;
   for (unsigned I = 0, E = NumVirtRegs; I != E; ++I) {
     Register VirtReg = Register::index2VirtReg(I);
-    if (MRI->getRegClass(VirtReg)->getID() == X86::TILERegClassID) {
+    if (!MRI->reg_nodbg_empty(VirtReg) &&
+        MRI->getRegClass(VirtReg)->getID() == X86::TILERegClassID) {
       HasVirtTileReg = true;
       break;
     }
diff --git a/llvm/test/CodeGen/X86/AMX/amx-fastpreconfig.mir b/llvm/test/CodeGen/X86/AMX/amx-fastpreconfig.mir
index dcc8d542c70704..40566520b79f01 100644
--- a/llvm/test/CodeGen/X86/AMX/amx-fastpreconfig.mir
+++ b/llvm/test/CodeGen/X86/AMX/amx-fastpreconfig.mir
@@ -59,3 +59,48 @@ body:             |
     RET 0, killed $eax
 
 ...
+# GlobalIsel doesn't use all virtual registers and there may be virtual
+# registers without a class.
+# Note that %3 doesn't have a class: gpr instead of gr64.
+---
+name:            test_unused
+legalized:       true
+regBankSelected: true
+selected:        true
+failedISel:      false
+tracksRegLiveness: true
+registers:
+  - { id: 0, class: gr64, preferred-register: '' }
+  - { id: 1, class: gr64_with_sub_8bit, preferred-register: '' }
+  - { id: 2, class: gr64, preferred-register: '' }
+  - { id: 3, class: gpr, preferred-register: '' }
+  - { id: 4, class: gr64, preferred-register: '' }
+  - { id: 5, class: gr8, preferred-register: '' }
+liveins:
+  - { reg: '$rdi', virtual-reg: '' }
+  - { reg: '$rsi', virtual-reg: '' }
+body:             |
+  bb.1.entry:
+    liveins: $rdi, $rsi
+
+    ; CHECK-LABEL: name: test_unused
+    ; CHECK: liveins: $rdi, $rsi
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: [[COPY:%[0-9]+]]:gr64 = COPY $rdi
+    ; CHECK-NEXT: [[COPY1:%[0-9]+]]:gr64_with_sub_8bit = COPY $rsi
+    ; CHECK-NEXT: [[COPY2:%[0-9]+]]:gr8 = COPY [[COPY1]].sub_8bit
+    ; CHECK-NEXT: $cl = COPY [[COPY2]]
+    ; CHECK-NEXT: [[SHR64rCL:%[0-9]+]]:gr64 = SHR64rCL [[COPY]], implicit-def $eflags, implicit $cl
+    ; CHECK-NEXT: [[ADD64ri32_:%[0-9]+]]:gr64 = ADD64ri32 [[SHR64rCL]], 123456789, implicit-def $eflags
+    ; CHECK-NEXT: $rax = COPY [[ADD64ri32_]]
+    ; CHECK-NEXT: RET 0, implicit $rax
+    %0:gr64 = COPY $rdi
+    %1:gr64_with_sub_8bit = COPY $rsi
+    %5:gr8 = COPY %1.sub_8bit
+    $cl = COPY %5
+    %2:gr64 = SHR64rCL %0, implicit-def $eflags, implicit $cl
+    %4:gr64 = ADD64ri32 %2, 123456789, implicit-def $eflags
+    $rax = COPY %4
+    RET 0, implicit $rax
+
+...
diff --git a/llvm/test/CodeGen/X86/domain-reassignment.mir b/llvm/test/CodeGen/X86/domain-reassignment.mir
index f4e454e3fa4974..29df0f70d6d75e 100644
--- a/llvm/test/CodeGen/X86/domain-reassignment.mir
+++ b/llvm/test/CodeGen/X86/domain-reassignment.mir
@@ -46,6 +46,10 @@
   define void @test_64bitext() #0 {
     ret void
   }
+  define void @test_globalisel_unused(i64 %0) #0 {
+    %unused = lshr i64 %0, 7
+    ret void
+  }
 ...
 ---
 name:            test_fcmp_storefloat
@@ -860,3 +864,63 @@ body:             |
     RET 0
 
 ...
+---
+name:            test_globalisel_unused
+alignment:       16
+exposesReturnsTwice: false
+legalized:       true
+regBankSelected: true
+selected:        true
+failedISel:      false
+tracksRegLiveness: true
+hasWinCFI:       false
+callsEHReturn:   false
+callsUnwindInit: false
+hasEHCatchret:   false
+hasEHScopes:     false
+hasEHFunclets:   false
+isOutlined:      false
+debugInstrRef:   false
+failsVerification: false
+tracksDebugUserValues: false
+registers:
+  - { id: 0, class: _, preferred-register: '' }
+  - { id: 1, class: _, preferred-register: '' }
+  - { id: 2, class: _, preferred-register: '' }
+liveins:
+  - { reg: '$rdi', virtual-reg: '' }
+frameInfo:
+  isFrameAddressTaken: false
+  isReturnAddressTaken: false
+  hasStackMap:     false
+  hasPatchPoint:   false
+  stackSize:       0
+  offsetAdjustment: 0
+  maxAlignment:    1
+  adjustsStack:    false
+  hasCalls:        false
+  stackProtector:  ''
+  functionContext: ''
+  maxCallFrameSize: 4294967295
+  cvBytesOfCalleeSavedRegisters: 0
+  hasOpaqueSPAdjustment: false
+  hasVAStart:      false
+  hasMustTailInVarArgFunc: false
+  hasTailCall:     false
+  localFrameSize:  0
+  savePoint:       ''
+  restorePoint:    ''
+fixedStack:      []
+stack:           []
+entry_values:    []
+callSites:       []
+debugValueSubstitutions: []
+constants:       []
+machineFunctionInfo: {}
+body:             |
+  bb.1 (%ir-block.1):
+    liveins: $rdi
+
+    RET 0
+
+...

@@ -860,3 +864,63 @@ body: |
RET 0

...
---
name: test_globalisel_unused
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the function named *globalisel*? It seems that there is not global isel flag in this test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even if it is present, it has no impact, as we run a single pass and MIR has been already selected. I want to show here that MIR for this function is obtained using GlobalISel, as a result we have registers without classes { id: 0, class: _, preferred-register: '' }. Yeah, I think more detailed comment will be better. Or do you think it may be even better to extract it to a separate file?

@@ -667,7 +667,8 @@ bool X86FastPreTileConfig::runOnMachineFunction(MachineFunction &MFunc) {
bool HasVirtTileReg = false;
for (unsigned I = 0, E = NumVirtRegs; I != E; ++I) {
Register VirtReg = Register::index2VirtReg(I);
if (MRI->getRegClass(VirtReg)->getID() == X86::TILERegClassID) {
if (!MRI->reg_nodbg_empty(VirtReg) &&
MRI->getRegClass(VirtReg)->getID() == X86::TILERegClassID) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use isTileRegisterClass?

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'm not sure. It's not a problem to replace == with TRI->isTileRegisterClass(MRI->getRegClass(VirtReg)) however we need to change const TargetRegisterInfo *TRI -> const X86RegisterInfo *TRI as well. The problem is that we have several static functions in this file where TRI is unavailable and we need to pass it through several calls. So, fixing this one place and leaving all others with explicit comparison don't make the situation much better. Initially they are at least consistent within the file. If we are interested in such refactor, I can do it separately.

Copy link
Contributor

@KanRobert KanRobert left a comment

Choose a reason for hiding this comment

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

LGTM

@e-kud e-kud merged commit cfd9119 into llvm:main Jan 26, 2024
4 checks passed
@e-kud e-kud deleted the global-amx2 branch March 8, 2024 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants