This patch implements the getInstrMapping hook for RISCVRegisterBankInfo and others in order to correctly select the GPR register bank for operands of ALU instructions, and the associated operations introduced by the legalizer.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| llvm/lib/Target/RISCV/RISCVRegisterBankInfo.cpp | ||
|---|---|---|
| 95 ↗ | (On Diff #400004) | I'd recommend keeping the subtarget cached in the class |
| 99 ↗ | (On Diff #400004) | I don't see the point of this loop. It seems like you're just doing a rough legality check, but the function is already required (and enforced by the verifier) to be legal at this point |
| 102 ↗ | (On Diff #400004) | You shouldn't see noreg on a generic instruction so I'm not sure what you're addressing here |
| llvm/test/CodeGen/RISCV/GlobalISel/regbankselect/alu32.mir | ||
| 7 ↗ | (On Diff #400004) | You can drop the IR section |
Comment Actions
Use size from LLT instead of from Subtarget.getXLen. This will make it easier to support D157770.
| llvm/lib/Target/RISCV/GISel/RISCVRegisterBankInfo.cpp | ||
|---|---|---|
| 124 | Should we consider adding new entries to ValueMappings so that we don't create this array locally? All the binary operations above don't create local arrays for this (though they could). | |
| 132 | Should we consider adding new entries to ValueMappings so that we don't create this array locally? All the binary operations above don't create local arrays for this (though they could). | |
| llvm/test/CodeGen/RISCV/GlobalISel/regbankselect/alu32.mir | ||
| 1 ↗ | (On Diff #551388) | Missing test for G_CONSTANT and G_ICMP |
| 6 ↗ | (On Diff #551388) | This test is identical to add_i32 below. |
| 51 ↗ | (On Diff #551388) | I don't think we need this test since we're separately testing for shl and ashr. |
| 78 ↗ | (On Diff #551388) | We don't have a test for G_CONSTANT, but if we make one, we may not need this test. We have tests for G_AND. |
| 286 ↗ | (On Diff #551388) | It's impossible to get a legal G_MUL on RISCV unless the +m or +zmmul attributes are enabled. It might be fine to have this test here to simply demonstrate how we select register banks for G_MUL. |
| 384 ↗ | (On Diff #551388) | I'm not sure we need separate tests for _i64. |
| llvm/test/CodeGen/RISCV/GlobalISel/regbankselect/alu64.mir | ||
| 1 ↗ | (On Diff #551388) | Missing test for G_CONSTANT and G_ICMP |
| 6 ↗ | (On Diff #551388) | Duplicate test, see add_i64 |
| 43 ↗ | (On Diff #551388) | I don't think we need this test since we're separately testing for shl and ashr. |
| 78 ↗ | (On Diff #551388) | Once we have a test for G_CONSTANT, we may not need this test. |
| 85 ↗ | (On Diff #551388) | All the _i32 tests are duplicates of the _i64 tests. |
| 664 ↗ | (On Diff #551388) | I don't think that the _i128 tests are meaningful, since we test for each of these generic opcodes separately. |
| llvm/lib/Target/RISCV/GISel/RISCVRegisterBankInfo.cpp | ||
|---|---|---|
| 124 | This implementation is consistent with ARM, AArch64, and Mips. I don't think it is possible to put a nullptr entry in the table. It needs to be a null RegisterBankInfo::ValueMapping* but the table row is an array of RegisterBankInfo::ValueMapping. | |
Comment Actions
Change back to using Subtarget to get register size. I think even with s32 being legal
on RV64 we still want to use a 64 bit size for the GPR.
Comment Actions
LGTM
| llvm/test/CodeGen/RISCV/GlobalISel/regbankselect/global-rv64.mir | ||
|---|---|---|
| 8 | Should this global be an i64? | |
| llvm/test/CodeGen/RISCV/GlobalISel/regbankselect/global-rv64.mir | ||
|---|---|---|
| 8 | The type doesn't matter. We're interested in the pointer to the variable not the type of the variable itself. The variable could be any type, even FP or a struct/array. | |