This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU/GlobalISel: Add support for simple shaders
ClosedPublic

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

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

getSubtarget<SISubtarget>

96

getSubtarget<SISubtarget>

lib/Target/AMDGPU/AMDGPUGenRegisterBankInfo.def
34

const

35

Typo Length

42

const

lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp
36

This should probably be the type of the subtarget to begin with

87–93

Can't the register bank be checked here directly to switch to v_add?

222

I'm not sure if Ptr == null is valid

293

const &

354–355

Define and declare at same type

391

This is probably noisy

392

Unused variable

lib/Target/AMDGPU/AMDGPUInstructionSelector.h
28

extra line

33

-virtual

48

ArrayRef?

51

ArrayRef?

lib/Target/AMDGPU/AMDGPURegisterBankInfo.h
26–28

Comments are leftover copy paste

lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
279

There's no reason to check this

281–283

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

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

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

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

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

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

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
240

'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 ↗(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
54

What do you mean?

ab added inline comments.Dec 15 2016, 1:15 PM
lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
54

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
54

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

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

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.

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