[PATCH] D148438: [AMDGPU] Do not crash on unknown register in AMDGPUResourceUsageAnalysis

[Pierre van Houtryve via Phabricator via llvm-commits]

Pierre-vh added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUResourceUsageAnalysis.cpp:486
           Width = 32;
-        } else {
-          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)) &&
-                 "Unknown register class");
         }
         unsigned HWReg = TRI.getHWRegIndex(Reg);
----------------
foad wrote:
> JonChesterfield wrote:
> > 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
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148438/new/

https://reviews.llvm.org/D148438