Some instruction selection patterns required for ALU GPR instructions have already been automatically imported from existing TableGen descriptions - this patch simply adds testing for them. Logic for selecting constants and copies has been added, along with the first of the GIComplexPatternEquiv definitions required to select the shiftMaskXLen ComplexPattern.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Unit Tests
Event Timeline
Comment Actions
Add custom selection for copies and for constants. Significantly expand tests over more types.
Comment Actions
Missing copy tests for all permutations of virtual and physical registers
| llvm/lib/Target/RISCV/RISCVInstructionSelector.cpp | ||
|---|---|---|
| 114–115 ↗ | (On Diff #258538) | This can't early return, it still needs to constrain a virtual source |
Comment Actions
Rebased. Added the first GIComplexPatternEquiv definition in order to handle the operands to shifts, which are now covered by a ComplexPattern in SelectionDAG. Updated the tests to match the output of the regbank selector.
| llvm/lib/Target/RISCV/RISCVInstructionSelector.cpp | ||
|---|---|---|
| 97–108 ↗ | (On Diff #400005) | This function name is misleading. Can you restructure to use something more like getRegClassForTypeOnBank (which AArch64 has a FIXME to make a generic TargetRegisterInfo method) |
Comment Actions
Restructure slightly, add more cases for selectCopy along with tests, add selection patterns for RV64 W instructions.
| llvm/lib/Target/RISCV/RISCVGISel.td | ||
|---|---|---|
| 30 ↗ | (On Diff #404931) | Should sext_inreg be legal? |
| llvm/lib/Target/RISCV/RISCVInstructionSelector.cpp | ||
| 46 ↗ | (On Diff #404931) | What is this parameter? |
| 109 ↗ | (On Diff #404931) | SrcReg.isPhysical() |
| 120 ↗ | (On Diff #404931) | Why not just print the whole instruction? |
| 145 ↗ | (On Diff #404931) | Should not construct one off MachineIRBuilders. The selector usually just directly uses BuildMI |
| 171 ↗ | (On Diff #404931) | Didn't erase the instruction |
| llvm/lib/Target/RISCV/RISCVGISel.td | ||
|---|---|---|
| 30 ↗ | (On Diff #404931) | This is what I'm stuck on right now. It really should to match the SelectionDAG code, at least for i32 -> i64. However there is no way to map G_SEXT_INREG to sext_inreg because sext_inreg uses a value type as a DAG operand, whereas G_SEXT_INREG uses a constant which indicates the size of the original type. So no patterns defined which use sext_inreg will work for GlobalISel. I think my only option left is to do custom selection code. |
| llvm/lib/Target/RISCV/RISCVGISel.td | ||
|---|---|---|
| 30 ↗ | (On Diff #404931) | You can either do custom selection code, or you could add support for sext_inreg in GlobalISelEmitter. It's a special case so it requires special support |
Comment Actions
Rebase. Still needs work.
Having s32 as a valid type in RV64 isn't working because GlobalISelEmitter doesn't really understand HwModes.
Comment Actions
Add test cases for addi, andi, ori, and xori.
Still need to work on slli(w), srli(w), srai(w), and addiw.
Comment Actions
Ok I think this is ready for review now. All the immediate cases are tested and working
| llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp | ||
|---|---|---|
| 104 ↗ | (On Diff #556102) | I assume we want to implement this TODO at higher optimization levels? |
| llvm/lib/Target/RISCV/RISCVGISel.td | ||
| 34 ↗ | (On Diff #556102) | Do we plan on implementing this earlier in a future patch? Why do we want to canonicalize it earlier? |
| llvm/test/CodeGen/RISCV/GlobalISel/alu-roundtrip.ll | ||
| 36 ↗ | (On Diff #556102) | I think this should be slliw? |
| 412 ↗ | (On Diff #556102) | Why is this instruction emitted? This looks like a nop. We may want to handle this in a later pass. |
| llvm/test/CodeGen/RISCV/GlobalISel/instruction-select/alu-rv32.mir | ||
| 3 ↗ | (On Diff #556102) | Do we want tests for and_i64, or_i64 and xor_i64? And immediate variants for these and add_i64, sub_i64? |
| llvm/test/CodeGen/RISCV/GlobalISel/alu-roundtrip.ll | ||
|---|---|---|
| 36 ↗ | (On Diff #556102) | The upper bits aren’t used by the sraiw so SLLI should be ok |
| 412 ↗ | (On Diff #556102) | The upper half immediate is 0 and we don’t have combines enabled. |
| llvm/test/CodeGen/RISCV/GlobalISel/instruction-select/alu-rv32.mir | ||
| 3 ↗ | (On Diff #556102) | I don’t think they provide any value. They would all be split instructions. |
| llvm/lib/Target/RISCV/RISCVGISel.td | ||
|---|---|---|
| 34 ↗ | (On Diff #556102) | I was planning to add a post legalizer combine for it. |
| llvm/test/CodeGen/RISCV/GlobalISel/alu-roundtrip.ll | ||
|---|---|---|
| 36 ↗ | (On Diff #556102) | To clarify, the change from slliw to slli was made by the RISCVOptWInstrs pass. |