Page MenuHomePhabricator

AMDGPU/GlobalISel: Add support for simple shaders
ClosedPublic

Authored by tstellarAMD on Nov 15 2016, 6:48 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

tstellarAMD retitled this revision from to AMDGPU/GlobalISel: Add support for simple shaders.
tstellarAMD updated this object.
tstellarAMD added a subscriber: llvm-commits.
MatzeB resigned from this revision.Nov 18 2016, 4:22 PM
MatzeB removed a reviewer: MatzeB.
arsenm added inline comments.Nov 18 2016, 4:28 PM
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

qcolombet edited edge metadata.Nov 30 2016, 3:27 PM

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 :).

tstellarAMD marked 17 inline comments as done.
tstellarAMD edited edge metadata.

Address some review comments.

tstellarAMD added inline comments.Dec 2 2016, 3:40 PM
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.

ab added inline comments.Dec 15 2016, 11:12 AM
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?

arsenm added inline comments.Dec 15 2016, 11:54 AM
lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
53 ↗(On Diff #80141)

What do you mean?

ab added inline comments.Dec 15 2016, 1:15 PM
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?

arsenm added inline comments.Dec 15 2016, 1:22 PM
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

arsenm added inline comments.Dec 15 2016, 1:26 PM
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

tstellarAMD marked 10 inline comments as done.

Address review comments.

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.

Rebase on top of master.

ab accepted this revision.Jan 24 2017, 6:12 PM

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
This revision is now accepted and ready to land.Jan 24 2017, 6:12 PM
This revision was automatically updated to reflect the committed changes.