We can select constant/global G_LOAD, global G_STORE, and G_GEP.
Details
- Reviewers
qcolombet t.p.northover ab arsenm - Commits
- rGca16621b2a0e: Re-commit AMDGPU/GlobalISel: Add support for simple shaders
rGe48f60aec856: AMDGPU/GlobalISel: Add support for simple shaders
rL293551: Re-commit AMDGPU/GlobalISel: Add support for simple shaders
rL293503: AMDGPU/GlobalISel: Add support for simple shaders
Diff Detail
- Repository
- rL LLVM
Event Timeline
| lib/Target/AMDGPU/AMDGPUCallLowering.cpp | ||
|---|---|---|
| 49 ↗ | (On Diff #78127) | getSubtarget<SISubtarget> |
| 96 ↗ | (On Diff #78127) | getSubtarget<SISubtarget> |
| lib/Target/AMDGPU/AMDGPUGenRegisterBankInfo.def | ||
| 34 ↗ | (On Diff #78127) | const |
| 35 ↗ | (On Diff #78127) | Typo Length |
| 42 ↗ | (On Diff #78127) | const |
| lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp | ||
| 36 ↗ | (On Diff #78127) | This should probably be the type of the subtarget to begin with |
| 87–93 ↗ | (On Diff #78127) | Can't the register bank be checked here directly to switch to v_add? |
| 222 ↗ | (On Diff #78127) | I'm not sure if Ptr == null is valid |
| 293 ↗ | (On Diff #78127) | const & |
| 354–355 ↗ | (On Diff #78127) | Define and declare at same type |
| 391 ↗ | (On Diff #78127) | This is probably noisy |
| 392 ↗ | (On Diff #78127) | Unused variable |
| lib/Target/AMDGPU/AMDGPUInstructionSelector.h | ||
| 28 ↗ | (On Diff #78127) | extra line |
| 33 ↗ | (On Diff #78127) | -virtual |
| 48 ↗ | (On Diff #78127) | ArrayRef? |
| 51 ↗ | (On Diff #78127) | ArrayRef? |
| lib/Target/AMDGPU/AMDGPURegisterBankInfo.h | ||
| 26–28 ↗ | (On Diff #78127) | Comments are leftover copy paste |
| lib/Target/AMDGPU/AMDGPUTargetMachine.cpp | ||
| 279 ↗ | (On Diff #78127) | There's no reason to check this |
| 281–283 ↗ | (On Diff #78127) | RBI should be created first direct to the reset, then you can avoid the temporary naked pointer |
Hi Tom,
From bringing up the GlobalISel infrastructure point of view, this looks good.
I would recommend to add test cases for the different passes using .mir instead of just having the checks for full pipeline (.ll -> asm).
Cheers,
-Quentin
| lib/Target/AMDGPU/AMDGPUCallLowering.cpp | ||
|---|---|---|
| 88 ↗ | (On Diff #78127) | I would add a boolean and return false when we encounter such case. At least an assert seems in order :). |
| lib/Target/AMDGPU/AMDGPUCallLowering.cpp | ||
|---|---|---|
| 88 ↗ | (On Diff #78127) | We switched to a new ABI since I started working on this, so I don't think we need to handle this case any more, so I'll drop the comment. |
| lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp | ||
| 87–93 ↗ | (On Diff #78127) | Yes, we can but we AMDGPURegisterBankInfo doesn't handle GEP/ADD with VGPRs yet, so the instruction selector will only ever see GEP/ADD with SGPRs. |
| 222 ↗ | (On Diff #78127) | It can be null if there is a PseudoSourceValue and you can construct an MMO with a nullptr value too, which I think we used to do. |
| lib/Target/AMDGPU/AMDGPUCallLowering.cpp | ||
|---|---|---|
| 1 ↗ | (On Diff #80141) | Is this line 1-char short? |
| 38 ↗ | (On Diff #80141) | Indentation. |
| 39 ↗ | (On Diff #80141) | Is the returned vreg ignored because all functions are void? |
| lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp | ||
| 1144–1145 ↗ | (On Diff #80141) | Unrelated change? |
| lib/Target/AMDGPU/AMDGPUISelLowering.h | ||
| 216 ↗ | (On Diff #80141) | FWIW I'd keep this in CallLowering (maybe hardcoded?), as you're not already using CCAssignFn in ISelLowering anyway. |
| lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp | ||
| 36–37 ↗ | (On Diff #80141) | Re-indent? |
| 40 ↗ | (On Diff #80141) | Indentation. |
| 184 ↗ | (On Diff #80141) | Unnecessary braces. |
| lib/Target/AMDGPU/AMDGPUInstructionSelector.h | ||
| 18 ↗ | (On Diff #80141) | Forward-decl MachineInstr/MachineOperand too? |
| lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp | ||
| 53 ↗ | (On Diff #80141) | Huh, why are non-pointer types legal? |
| lib/Target/AMDGPU/AMDGPURegisterBankInfo.h | ||
| 43–48 ↗ | (On Diff #80141) | Remove the redundant comments? (I fixed AArch64 in r289844) |
| lib/Target/AMDGPU/AMDGPUTargetMachine.cpp | ||
| 241 ↗ | (On Diff #80141) | 'class' isn't necessary anymore. Also fixed the other targets in r289848 ;) |
| lib/Target/AMDGPU/CMakeLists.txt | ||
| 42 ↗ | (On Diff #80141) | You should only includes these files if LLVM_BUILD_GLOBAL_ISEL=ON. AArch64/CMakeLists.txt has an example of the cmake goop you'll need. |
| lib/Target/AMDGPU/SIISelLowering.cpp | ||
| 615 ↗ | (On Diff #80141) | Maybe commit this and the InstrInfo changes separately? |
| lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp | ||
|---|---|---|
| 53 ↗ | (On Diff #80141) | What do you mean? |
| lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp | ||
|---|---|---|
| 53 ↗ | (On Diff #80141) | Why is it legal to have an 's64' pointer (#1) operand to G_STORE/G_LOAD/G_GEP? |
| lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp | ||
|---|---|---|
| 53 ↗ | (On Diff #80141) | Oh, I didn't realize integers and pointers were distinct here. 32-bit and 64-bit pointers should both be legal |
| lib/Target/AMDGPU/AMDGPUCallLowering.cpp | ||
|---|---|---|
| 39 ↗ | (On Diff #80141) | Yes. For now only some graphics shaders are allowed to have return values which aren't necessary for the initial implementation |
| lib/Target/AMDGPU/AMDGPUISelLowering.h | ||
|---|---|---|
| 216 ↗ | (On Diff #80141) | I moved the function to the CallLowering class, but I let the implementation in the AMDGPUISelLowering file, because CC_AMDGPU is defined in AMDGPUGenCallingConv.inc |
| lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp | ||
| 53 ↗ | (On Diff #80141) | RegBankSelect will only create copies with scalar types, so if the pointer operands need to be copied to a new register bank, the new value will have a scalar type and not a pointer type. |
I'd recommend splitting out a few tests for the hairier parts of RBS, and you folks know the target specifics, but otherwise, this looks good to me.
| lib/Target/AMDGPU/AMDGPUInstructionSelector.h | ||
|---|---|---|
| 39 ↗ | (On Diff #85584) | Also private? |
| lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp | ||
| 53 ↗ | (On Diff #80141) | Huh, ..that's a bug. In particular, it means that mapping a vector operation to two parts will lose the vector type, and that's plain incorrect. We'll have to look into that; expressing the mappings in terms of types instead of size should be sufficient. |
| lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp | ||
| 142 ↗ | (On Diff #85584) | You can getSizeInBits() on the type of the pointer operand as well; they always have both addressspace and size. |
| lib/Target/AMDGPU/AMDGPURegisterBanks.td | ||
| 9–11 ↗ | (On Diff #85584) | Add a comment, or remove the block? |
| test/CodeGen/AMDGPU/GlobalISel/inst-select-load-flat.mir | ||
| 13–15 ↗ | (On Diff #85584) | This isn't necessary anymore: you can now put the bank/class at the first definition: %0:vgpr(p1) = COPY %vgpr0_vgpr1 |