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 2352 Build 2352: arc lint + arc unit
Event Timeline
lib/Target/AMDGPU/AMDGPUCallLowering.cpp | ||
---|---|---|
49 | getSubtarget<SISubtarget> | |
93 | 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 | ||
297 | There's no reason to check this | |
299–301 | 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–39 | Indentation. | |
39 | 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 | ||
37–38 | Re-indent? | |
41 | Indentation. | |
185 | Unnecessary braces. | |
lib/Target/AMDGPU/AMDGPUInstructionSelector.h | ||
19 | Forward-decl MachineInstr/MachineOperand too? | |
lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp | ||
54 | Huh, why are non-pointer types legal? | |
lib/Target/AMDGPU/AMDGPURegisterBankInfo.h | ||
44–49 | Remove the redundant comments? (I fixed AArch64 in r289844) | |
lib/Target/AMDGPU/AMDGPUTargetMachine.cpp | ||
257 | 'class' isn't necessary anymore. Also fixed the other targets in r289848 ;) | |
lib/Target/AMDGPU/CMakeLists.txt | ||
45 | 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 | ||
---|---|---|
54 | What do you mean? |
lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp | ||
---|---|---|
54 | Why is it legal to have an 's64' pointer (#1) operand to G_STORE/G_LOAD/G_GEP? |
lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp | ||
---|---|---|
54 | 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 ↗ | (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 | ||
54 | 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 | ||
54 | 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?