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 |