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
- Build Status
Buildable 1827 Build 1827: arc lint + arc unit
Event Timeline
| lib/Target/AMDGPU/AMDGPUCallLowering.cpp | ||
|---|---|---|
| 49 | getSubtarget<SISubtarget> | |
| 94 | getSubtarget<SISubtarget> | |
| lib/Target/AMDGPU/AMDGPUGenRegisterBankInfo.def | ||
| 35 | const | |
| 36 | Typo Length | |
| 43 | const | |
| lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp | ||
| 37 | This should probably be the type of the subtarget to begin with | |
| 88–94 | Can't the register bank be checked here directly to switch to v_add? | |
| 223 | I'm not sure if Ptr == null is valid | |
| 294 | const & | |
| 355–356 | Define and declare at same type | |
| 392 | This is probably noisy | |
| 393 | Unused variable | |
| lib/Target/AMDGPU/AMDGPUInstructionSelector.h | ||
| 29 | extra line | |
| 34 | -virtual | |
| 49 | ArrayRef? | |
| 52 | ArrayRef? | |
| lib/Target/AMDGPU/AMDGPURegisterBankInfo.h | ||
| 27–29 | Comments are leftover copy paste | |
| lib/Target/AMDGPU/AMDGPUTargetMachine.cpp | ||
| 280 | There's no reason to check this | |
| 282–284 | 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 | 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 | 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 | ||
| 88–94 | 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. | |
| 223 | 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 | Is this line 1-char short? | |
| 38 | Indentation. | |
| 39 | Is the returned vreg ignored because all functions are void? | |
| lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp | ||
| 1144–1145 | Unrelated change? | |
| lib/Target/AMDGPU/AMDGPUISelLowering.h | ||
| 216 | 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 | Re-indent? | |
| 40 | Indentation. | |
| 184 | Unnecessary braces. | |
| lib/Target/AMDGPU/AMDGPUInstructionSelector.h | ||
| 18 | Forward-decl MachineInstr/MachineOperand too? | |
| lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp | ||
| 53 | Huh, why are non-pointer types legal? | |
| lib/Target/AMDGPU/AMDGPURegisterBankInfo.h | ||
| 43–48 | Remove the redundant comments? (I fixed AArch64 in r289844) | |
| lib/Target/AMDGPU/AMDGPUTargetMachine.cpp | ||
| 241 | 'class' isn't necessary anymore. Also fixed the other targets in r289848 ;) | |
| lib/Target/AMDGPU/CMakeLists.txt | ||
| 42 | 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 | Maybe commit this and the InstrInfo changes separately? | |
| lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp | ||
|---|---|---|
| 53 | What do you mean? | |
| lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp | ||
|---|---|---|
| 53 | Why is it legal to have an 's64' pointer (#1) operand to G_STORE/G_LOAD/G_GEP? | |
| lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp | ||
|---|---|---|
| 53 | 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 | 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 | 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 | 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 | ||
|---|---|---|
| 40 | Also private? | |
| lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp | ||
| 53 | 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 | ||
| 143 | 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 | ||
| 14–16 | This isn't necessary anymore: you can now put the bank/class at the first definition: %0:vgpr(p1) = COPY %vgpr0_vgpr1 | |
Is this line 1-char short?