[AMDGPU] Do not crash on agpr_hi16 in AMDGPUResourceUsageAnalysis

[foad]

llvm/test/CodeGen/AMDGPU/resource-usage-agpr-hi16.ll

42 ↗

(On Diff #513932)

Note that $agpr0_hi16 is defined but there is no AGPR_HI16 register class.

These mentions of AGPR hi16 regs were introduced by BranchFolderPass.

[Pierre-vh]

There's a test failure, otherwise LGTM so I'll approve when it passes

llvm/test/CodeGen/AMDGPU/resource-usage-agpr-hi16.ll

42 ↗

(On Diff #513932)

small nit: as we don't care about most of the output, perhaps just check for that line only instead of generating the whole test?
I would also add a comment to the test to explain that we're checking that agpr_16 doesn't crash resource usage analysis because it doesn't have an associated RC.

[foad]

Ungenerate test checks.

[Pierre-vh]

LGTM if the test passes

[foad]

An alternative would be to fix/optimize the branch folder. Currently BranchFolder::mergeCommonTails adds IMPLICIT_DEFs based on the regs computed by LivePhysRegs:computeLiveIns before calling addLiveIns, which means it lacks the "Skip the register if we are about to add one of its super registers" optimization from addLiveIns and adds redundant IMPLICIT_DEFs of subregs. But I haven't found a neat way to implement this yet.

[foad]

Add a TODO.

[Pierre-vh]

In D148438#4273268, @foad wrote:

An alternative would be to fix/optimize the branch folder. Currently BranchFolder::mergeCommonTails adds IMPLICIT_DEFs based on the regs computed by LivePhysRegs:computeLiveIns before calling addLiveIns, which means it lacks the "Skip the register if we are about to add one of its super registers" optimization from addLiveIns and adds redundant IMPLICIT_DEFs of subregs. But I haven't found a neat way to implement this yet.

This sounds like it definitely needs fixing, but I feel like this is a separate issue. Resource usage analysis shouldn't crash on valid code, though i guess the definition of "valid" is quite loose here - agpr_hi16 isn't addressable I think so it's not exactly valid, but it doesn't crash at emission so I would say there's no reason for resource usage analysis to crash.

[Pierre-vh]

Take over patch

[foad]

LGTM but you should probably get an independent reviewer.

[JonChesterfield]

llvm/lib/Target/AMDGPU/AMDGPUResourceUsageAnalysis.cpp

496

The structure above the assert definitely looks error prone, but on a glance it seems plausible that we should be adding the missing cases for agpr_lo16 and keeping the assert

[foad]

llvm/lib/Target/AMDGPU/AMDGPUResourceUsageAnalysis.cpp

496

We can't easily add AMDGPU::AGPR_HI16RegClass.contains(Reg) above because there is no AGPR_HI16 register class. I guess we could define one, but that seems like an invasive change just to avoid an assertion failure here.

[Pierre-vh]

llvm/lib/Target/AMDGPU/AMDGPUResourceUsageAnalysis.cpp

496

I'm also not a fan of asserting on non ill-formed MIR. I think this assert is intended to catch missing/uncovered RCs but I don't think this is the right way to do it as lit tests aren't guaranteed to catch it.

If it's really important to check that this pass doesn't miss any register (class), IMO the logic should be moved into a helper w/ a unit test that iterates over all RCs instead so it catches missing cases much earlier.

[rampitec]

llvm/lib/Target/AMDGPU/AMDGPUResourceUsageAnalysis.cpp

496

You have TargetRegisterInfo here, so you may keep the assert and complement it with the check || !getPhysRegBaseClass(Reg). I.e. the check that the register does not belong to any RC.

[Pierre-vh]

Add the assert back in

llvm/lib/Target/AMDGPU/AMDGPUResourceUsageAnalysis.cpp

for (const MachineInstr &MI : MBB) {
         } else if (AMDGPU::VReg_1024RegClass.contains(Reg)) {
           IsSGPR = false;
           Width = 32;
         } else if (AMDGPU::AReg_1024RegClass.contains(Reg)) {
           IsSGPR = false;
           IsAGPR = true;
           Width = 32;
         } else {
+          // We only expect TTMP registers or registers that do not belong to
+          // any RC.
           assert((AMDGPU::TTMP_32RegClass.contains(Reg) ||
                   AMDGPU::TTMP_64RegClass.contains(Reg) ||
                   AMDGPU::TTMP_128RegClass.contains(Reg) ||
                   AMDGPU::TTMP_256RegClass.contains(Reg) ||
-                  AMDGPU::TTMP_512RegClass.contains(Reg)) &&
+                  AMDGPU::TTMP_512RegClass.contains(Reg) ||
+                  !TRI.getPhysRegBaseClass(Reg)) &&
                  "Unknown register class");
         }
         unsigned HWReg = TRI.getHWRegIndex(Reg);
         int MaxUsed = HWReg + Width - 1;
         if (IsSGPR) {
           MaxSGPR = MaxUsed > MaxSGPR ? MaxUsed : MaxSGPR;
         } else if (IsAGPR) {
           MaxAGPR = MaxUsed > MaxAGPR ? MaxUsed : MaxAGPR;
         } else {
           MaxVGPR = MaxUsed > MaxVGPR ? MaxUsed : MaxVGPR;

llvm/test/CodeGen/AMDGPU/resource-usage-crash-unhandled-reg.mir

+# RUN: llc -march=amdgcn -mcpu=gfx90a -run-pass=amdgpu-resource-usage -verify-machineinstrs %s -o -
+
+# Checks that ResourceUsageAnalysis does not assert if it sees agpr_lo16, agpr_hi16 or TTMP regs.
+---
+
+name: test
+tracksRegLiveness: true
+frameInfo:
+  hasCalls: true
+body: |
+  bb.0:
+    liveins: $sgpr0_sgpr1, $ttmp0, $ttmp0_ttmp1, $ttmp0_ttmp1_ttmp2_ttmp3
+    $vgpr0 = COPY $ttmp0
+    $vgpr1_vgpr2 = COPY $ttmp0_ttmp1
+    $vgpr1_vgpr2_vgpr3_vgpr4 = COPY $ttmp0_ttmp1_ttmp2_ttmp3
+    $vgpr1_vgpr2_vgpr3_vgpr4_vgpr5_vgpr6_vgpr7_vgpr8 = COPY $ttmp0_ttmp1_ttmp2_ttmp3_ttmp4_ttmp5_ttmp6_ttmp7
+    $vgpr1_vgpr2_vgpr3_vgpr4_vgpr5_vgpr6_vgpr7_vgpr8_vgpr9_vgpr10_vgpr11_vgpr12_vgpr13_vgpr14_vgpr15_vgpr16 = COPY $ttmp0_ttmp1_ttmp2_ttmp3_ttmp4_ttmp5_ttmp6_ttmp7_ttmp8_ttmp9_ttmp10_ttmp11_ttmp12_ttmp13_ttmp14_ttmp15
+    $agpr1_lo16 = IMPLICIT_DEF
+    $agpr1_hi16 = IMPLICIT_DEF
+    $sgpr2_sgpr3 = SI_CALL undef $sgpr0_sgpr1, 0, CustomRegMask()
+    S_ENDPGM 0
+...