[AMDGPU] Override PSet for M0

[rampitec]

Summary

This change returns empty PSet list for M0 register. Otherwise its
PSet as defined by tablegen is SReg_32. This results in incorrect
register pressure calculation every time an instruction uses M0.
Such uses count as SReg_32 PSet and inadequately increase pressure
on SGPRs.

[arsenm]

lib/Target/AMDGPU/SIRegisterInfo.cpp

1336–1337 ↗

(On Diff #87910)

Is == M0 sufficient? m0 should be its own regunit since it can't be a subregister

test/CodeGen/AMDGPU/schedule-regpressure.mir

1 ↗

(On Diff #87910)

This is going to break in a release build

1 ↗

(On Diff #87910)

Better would be a test that doesn't rely on debug output, but if that's not possible you need REQUIRES: asserts or something like that

[rampitec]

Added # REQUIRES: asserts to the test.

[rampitec]

lib/Target/AMDGPU/SIRegisterInfo.cpp

1336–1337 ↗

(On Diff #87910)

Yes, it is in its own RegUnit, and RegUnit is what we get here. This test yields true for M0 and only for M0 because it does not share a RegUnit with any other register.

[arsenm]

lib/Target/AMDGPU/SIRegisterInfo.cpp

1336–1337 ↗

(On Diff #87910)

I mean it's simpler to do just return RegUnit == AMDGPU::M0 instead of doing the whole regunit iterator test

[rampitec]

lib/Target/AMDGPU/SIRegisterInfo.cpp

1336–1337 ↗

(On Diff #87910)

AMDGPU::M0 == 89, its RegUnit == 81.

[rampitec]

lib/Target/AMDGPU/SIRegisterInfo.cpp

1336–1337 ↗

(On Diff #87910)

Here, gdb log:

(gdb) p (int)AMDGPU::M0
$9 = 89
(gdb) call TRI->hasRegUnit(AMDGPU::M0, 81)
$12 = true
(gdb) call TRI->hasRegUnit(AMDGPU::M0, 89)
$13 = false
(gdb) call TRI->hasRegUnit(AMDGPU::M0, AMDGPU::M0)
$14 = false

[arsenm]

LGTM with param name fixed

lib/Target/AMDGPU/SIRegisterInfo.cpp

1333 ↗

(On Diff #87919)

Ditto

lib/Target/AMDGPU/SIRegisterInfo.h

214 ↗

(On Diff #87919)

Param name is misleading, it should be Reg not RegUnit

[rampitec]

lib/Target/AMDGPU/SIRegisterInfo.h

214 ↗

(On Diff #87919)

Matt, this is RegUnit. If that would be reg I would not need to call hasRegUnit and just compared register directly. The function is called getRegUnit... and its parent definition has RegUnit argument name.

It is a RegUnit, really ;)

[arsenm]

lib/Target/AMDGPU/SIRegisterInfo.h

214 ↗

(On Diff #87919)

OK, I really hate the RegUnit distinctions. This confusion happens everywhere

llvm/trunk/lib/Target/AMDGPU/SIRegisterInfo.h

bool shouldCoalesce(MachineInstr *MI,
                       const TargetRegisterClass *NewRC) const override;
 
   unsigned getRegPressureLimit(const TargetRegisterClass *RC,
                                MachineFunction &MF) const override;
 
   unsigned getRegPressureSetLimit(const MachineFunction &MF,
                                   unsigned Idx) const override;
 
+  const int *getRegUnitPressureSets(unsigned RegUnit) const override;
+
 private:
   void buildSpillLoadStore(MachineBasicBlock::iterator MI,
                            unsigned LoadStoreOp,
                            int Index,
                            unsigned ValueReg,
                            bool ValueIsKill,
                            unsigned ScratchRsrcReg,
                            unsigned ScratchOffsetReg,
                            int64_t InstrOffset,
                            MachineMemOperand *MMO,
                            RegScavenger *RS) const;
 };
 
 } // End namespace llvm
 
 #endif

llvm/trunk/lib/Target/AMDGPU/SIRegisterInfo.cpp

return getRegPressureLimit(&AMDGPU::VGPR_32RegClass,
                                const_cast<MachineFunction &>(MF));
 
   if (Idx == getSGPRPressureSet())
     return getRegPressureLimit(&AMDGPU::SGPR_32RegClass,
                                const_cast<MachineFunction &>(MF));
 
   return AMDGPURegisterInfo::getRegPressureSetLimit(MF, Idx);
 }
+
+const int *SIRegisterInfo::getRegUnitPressureSets(unsigned RegUnit) const {
+  static const int Empty[] = { -1 };
+
+  if (hasRegUnit(AMDGPU::M0, RegUnit))
+    return Empty;
+  return AMDGPURegisterInfo::getRegUnitPressureSets(RegUnit);
+}

llvm/trunk/test/CodeGen/AMDGPU/schedule-regpressure.mir

+# RUN: llc -march=amdgcn -misched=converge -run-pass machine-scheduler %s -o - -debug-only=misched 2>&1 | FileCheck %s
+# REQUIRES: asserts
+
+# Check there is no SReg_32 pressure created by DS_* instructions because of M0 use
+
+# CHECK: ScheduleDAGMILive::schedule starting
+# CHECK: SU({{.*}} = DS_READ_B32 {{.*}} %M0<imp-use>, %EXEC<imp-use>
+# CHECK: Pressure Diff : {{$}}
+# CHECK: SU({{.*}} DS_WRITE_B32
+
+---
+name:            mo_pset
+alignment:       0
+exposesReturnsTwice: false
+legalized:       false
+regBankSelected: false
+selected:        false
+tracksRegLiveness: true
+registers:
+  - { id: 0, class: sreg_128 }
+  - { id: 1, class: sgpr_64 }
+  - { id: 2, class: sreg_32_xm0 }
+  - { id: 3, class: sgpr_32 }
+  - { id: 4, class: vgpr_32 }
+  - { id: 5, class: sreg_32_xm0_xexec }
+  - { id: 6, class: vgpr_32 }
+  - { id: 7, class: vgpr_32 }
+  - { id: 8, class: vgpr_32 }
+liveins:
+  - { reg: '%sgpr4_sgpr5', virtual-reg: '%1' }
+frameInfo:
+  isFrameAddressTaken: false
+  isReturnAddressTaken: false
+  hasStackMap:     false
+  hasPatchPoint:   false
+  stackSize:       0
+  offsetAdjustment: 0
+  maxAlignment:    0
+  adjustsStack:    false
+  hasCalls:        false
+  maxCallFrameSize: 0
+  hasOpaqueSPAdjustment: false
+  hasVAStart:      false
+  hasMustTailInVarArgFunc: false
+body:             |
+  bb.0:
+    liveins: %sgpr4_sgpr5
+
+    %1 = COPY %sgpr4_sgpr5
+    %5 = S_LOAD_DWORD_IMM %1, 0, 0 :: (non-temporal dereferenceable invariant load 4 from `i32 addrspace(2)* undef`)
+    %m0 = S_MOV_B32 -1
+    %7 = COPY %5
+    %6 = DS_READ_B32 %7, 0, 0, implicit %m0, implicit %exec
+    DS_WRITE_B32 %7, %6, 4, 0, implicit killed %m0, implicit %exec
+    S_ENDPGM
+
+...