We can select constant/global G_LOAD, global G_STORE, and G_GEP.
qcolombet t.p.northover ab arsenm
- 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
This should probably be the type of the subtarget to begin with
Can't the register bank be checked here directly to switch to v_add?
I'm not sure if Ptr == null is valid
Define and declare at same type
This is probably noisy
Comments are leftover copy paste
There's no reason to check this
RBI should be created first direct to the reset, then you can avoid the temporary naked pointer
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).
I would add a boolean and return false when we encounter such case. At least an assert seems in order :).
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.
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.
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.
Is this line 1-char short?
Is the returned vreg ignored because all functions are void?
|1144–1145 ↗||(On Diff #80141)|
FWIW I'd keep this in CallLowering (maybe hardcoded?), as you're not already using CCAssignFn in ISelLowering anyway.
Forward-decl MachineInstr/MachineOperand too?
Huh, why are non-pointer types legal?
Remove the redundant comments? (I fixed AArch64 in r289844)
'class' isn't necessary anymore. Also fixed the other targets in r289848 ;)
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.
|615 ↗||(On Diff #80141)|
Maybe commit this and the InstrInfo changes separately?
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
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.
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.
You can getSizeInBits() on the type of the pointer operand as well; they always have both addressspace and size.
|9–11 ↗||(On Diff #85584)|
Add a comment, or remove the block?
|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