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

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec 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);
----------------
Pierre-vh wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148438