This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Remove ImagePSV and move images to addrspace 7
ClosedPublic

Authored by nhaehnle on Nov 29 2022, 1:47 PM.

Details

Summary

Following up on the removal of BufferPSV in commit 43b86bf992 ("AMDGPU:
Remove BufferPseudoSourceValue")

It is unclear what exactly the right address space for images should be.
They seem morally closest to buffers, so that's what I went with. In
practical terms, address space 7 is better than address space 0 because
it can't alias with LDS.

Diff Detail

Unit TestsFailed

Event Timeline

nhaehnle created this revision.Nov 29 2022, 1:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 29 2022, 1:47 PM
nhaehnle requested review of this revision.Nov 29 2022, 1:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 29 2022, 1:47 PM
Herald added a subscriber: wdng. · View Herald Transcript
nhaehnle updated this revision to Diff 478722.Nov 29 2022, 2:12 PM

Update some test cases I had missed

foad accepted this revision.Nov 30 2022, 1:41 AM

I like it.

llvm/lib/Target/AMDGPU/SIISelLowering.cpp
995

The braces are no longer needed.

llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.h
385

Are you planning to remove GWSResourcePSV too?

This revision is now accepted and ready to land.Nov 30 2022, 1:41 AM

Thanks!

llvm/lib/Target/AMDGPU/SIISelLowering.cpp
995

Sure.

llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.h
385

No. That's simply because I haven't thought enough about it, don't really have the time, and it feels like enough of a rare and specialized corner that it's fine to leave it as-is. I also wouldn't want to commit to clearing out all mention of PSV's from the backend just yet. @arsenm may want to weigh in more.

This revision was landed with ongoing or failed builds.Nov 30 2022, 2:32 AM
This revision was automatically updated to reflect the committed changes.