This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: AMDPAL scratch buffer support
ClosedPublic

Authored by tpr on Sep 5 2017, 9:48 AM.

Details

Summary

(From TimC, with further modifications.)

Added support for scratch (including spilling) for OS type amdpal:
generates code to set up the scratch descriptor if it is needed.

With amdpal, the scratch resource descriptor is loaded from offset 0 of
the global information table. The low 32 bits of the address of the
global information table is passed in s0.

Added amdgpu-git-ptr-high function attribute to hard-wire the high 32
bits of the address of the global information table. If the function
attribute is not specified, or is 0xffffffff, then the backend generates
code to use the high 32 bits of pc.

The documentation for the AMDPAL ABI will be added in a later commit.

Event Timeline

tpr created this revision.Sep 5 2017, 9:48 AM
tpr added a subscriber: llvm-commits.
arsenm added inline comments.Sep 5 2017, 10:12 AM
lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp
920

I don't think we actually care what order these are printed in. Can you just move all of these to one place?

lib/Target/AMDGPU/SIFrameLowering.cpp
358–416

This looks like it breaks mesa

395–396

Sort the add MMO to last

lib/Target/AMDGPU/SIMachineFunctionInfo.cpp
163–164

You can combine these and check if it's empty

test/CodeGen/AMDGPU/amdpal.ll
11

-enable-var-scope on new tests

16

mising -LABEL

arsenm added inline comments.Sep 5 2017, 10:14 AM
lib/Target/AMDGPU/SIFrameLowering.cpp
358–416

Oh, I see that just moved. Can you maybe extract this + the mesa setup to a separate function?

tpr updated this revision to Diff 114699.Sep 11 2017, 2:25 PM

Addressed review comments. I'm a bit nervous though that the changes are now
more intrusive for the non-amdpal case, as I am not set up to test anything
other than amdpal.

tpr marked 6 inline comments as done.Sep 11 2017, 2:35 PM
arsenm added inline comments.Sep 15 2017, 10:28 AM
lib/Target/AMDGPU/SIFrameLowering.cpp
392

Why implicit def of s0? I don't think this should be necessary

tpr updated this revision to Diff 115623.Sep 18 2017, 4:35 AM

Addressed review comment.

tpr marked 2 inline comments as done.Sep 18 2017, 4:37 AM
arsenm accepted this revision.Sep 19 2017, 11:01 AM

LGTM

lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp
899

This looks like it's already here, but while you're here can you remove this check in a separate patch?

This revision is now accepted and ready to land.Sep 19 2017, 11:01 AM
This revision was automatically updated to reflect the committed changes.