Page MenuHomePhabricator

[AMDGPU][CodeGen] Support raw format TFE buffer loads other than byte, short and d16 ones.
ClosedPublic

Authored by kosarev on Nov 17 2022, 7:44 AM.

Diff Detail

Event Timeline

kosarev created this revision.Nov 17 2022, 7:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 17 2022, 7:44 AM
kosarev requested review of this revision.Nov 17 2022, 7:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 17 2022, 7:44 AM
kosarev retitled this revision from [AMDGPU][CodeGen] Support raw TFE buffer loads other than byte, short and d16 ones. to [AMDGPU][CodeGen] Support raw format TFE buffer loads other than byte, short and d16 ones..Nov 17 2022, 7:46 AM

I thought TFE was already handled?

llvm/lib/Target/AMDGPU/BUFInstructions.td
1304–1305 ↗(On Diff #476133)

Why do these need fp and integer patterns? Given you've got the integer high element, I'd expect this to always be int vector

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.struct.buffer.load.format.ll
125 ↗(On Diff #476133)

All int and FP combinations not here?

149 ↗(On Diff #476133)

2 x case missing?

kosarev updated this revision to Diff 476414.Nov 18 2022, 3:36 AM

Removed unused patterns and added tests.

kosarev marked 3 inline comments as done.Nov 18 2022, 3:37 AM

I thought TFE was already handled?

Image TFE intrinsics were added in https://reviews.llvm.org/D48826, but TFE loads remained unsupported.

This revision was not accepted when it landed; it landed in state Needs Review.Nov 24 2022, 3:19 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
kosarev reopened this revision.Nov 24 2022, 3:39 AM

Apologies, this was unintentionally submitted with D138492.

LGTM, except I'd like to see tests trying to break the d16 interactions

llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.struct.buffer.load.format.ll
337 ↗(On Diff #477719)

Can you also add i16 and f16 cases?

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.struct.buffer.load.format.ll
124–125 ↗(On Diff #477719)

Tests should use opaque pointers

232 ↗(On Diff #477719)

Ditto, i16 and f16 cases

arsenm added inline comments.Nov 28 2022, 1:19 PM
llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
4557

If this isn't supposed to work, should return false for the fallback

arsenm requested changes to this revision.Dec 7 2022, 8:31 AM

Should test the 16-bit cases

This revision now requires changes to proceed.Dec 7 2022, 8:31 AM
kosarev updated this revision to Diff 482817.Dec 14 2022, 5:04 AM

Update tests to use opaque pointers.

kosarev marked an inline comment as done.Dec 14 2022, 5:10 AM
kosarev added inline comments.
llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
4557

What's the fallback? From what I see if we do not stop here, it will just crash later on a less specific check not being able to legalise the call.

llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.struct.buffer.load.format.ll
337 ↗(On Diff #477719)

The title says it that this is not supposed to support d16 cases. We would just crash on these.

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.struct.buffer.load.format.ll
232 ↗(On Diff #477719)

Answered above.

arsenm added inline comments.Dec 14 2022, 10:11 AM
llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
4557

The legalization error is a better user facing error than hitting the assert

kosarev updated this revision to Diff 483125.Dec 15 2022, 4:30 AM

Changed as requested.

kosarev marked an inline comment as done.Dec 15 2022, 4:30 AM
arsenm accepted this revision.Dec 15 2022, 8:08 AM
This revision is now accepted and ready to land.Dec 15 2022, 8:08 AM
arsenm accepted this revision.Dec 16 2022, 6:40 AM
This revision was landed with ongoing or failed builds.Dec 19 2022, 3:39 AM
This revision was automatically updated to reflect the committed changes.