Details
- Reviewers
foad arsenm rampitec - Group Reviewers
Restricted Project - Commits
- rGec82188451fc: [AMDGPU] Do not crash on agpr_hi16 in AMDGPUResourceUsageAnalysis
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
| llvm/test/CodeGen/AMDGPU/resource-usage-agpr-hi16.ll | ||
|---|---|---|
| 43 | Note that $agpr0_hi16 is defined but there is no AGPR_HI16 register class. These mentions of AGPR hi16 regs were introduced by BranchFolderPass. | |
There's a test failure, otherwise LGTM so I'll approve when it passes
| llvm/test/CodeGen/AMDGPU/resource-usage-agpr-hi16.ll | ||
|---|---|---|
| 43 | 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? | |
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.
| llvm/lib/Target/AMDGPU/AMDGPUResourceUsageAnalysis.cpp | ||
|---|---|---|
| 486 | 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 | |
| llvm/lib/Target/AMDGPU/AMDGPUResourceUsageAnalysis.cpp | ||
|---|---|---|
| 486 | 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. | |
| llvm/lib/Target/AMDGPU/AMDGPUResourceUsageAnalysis.cpp | ||
|---|---|---|
| 486 | 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. | |
| llvm/lib/Target/AMDGPU/AMDGPUResourceUsageAnalysis.cpp | ||
|---|---|---|
| 486 | 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. | |
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