[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

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

test/CodeGen/AMDGPU/schedule-regpressure.mir

2

This is going to break in a release build

2

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

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

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

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

[rampitec]

lib/Target/AMDGPU/SIRegisterInfo.cpp

1336–1337

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

Ditto

lib/Target/AMDGPU/SIRegisterInfo.h

214

Param name is misleading, it should be Reg not RegUnit

[rampitec]

lib/Target/AMDGPU/SIRegisterInfo.h

214

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

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

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

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);
+}

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
+
+...