Page MenuHomePhabricator

[AMDGPU] Add support for multi-dword s.buffer.load intrinsic
ClosedPublic

Authored by tpr on Aug 22 2018, 5:37 AM.

Details

Summary

Patch by Marek Olsen and David Stuttard, both of AMD.

This adds a new amdgcn intrinsic supporting s.buffer.load, in particular
multiple dword variants. These are convenient to use from some front-end
implementations.

Also modified the existing llvm.SI.load.const intrinsic to common up the
underlying implementation.

This modification also requires that we can lower to non-uniform loads correctly
by splitting larger dword variants into sizes supported by the non-uniform
versions of the load.

Change-Id: I83a6e00681158bb243591a94a51c7baa445f169b

Diff Detail

Repository
rL LLVM

Event Timeline

tpr created this revision.Aug 22 2018, 5:37 AM
arsenm added inline comments.Aug 22 2018, 10:29 AM
lib/Target/AMDGPU/SIInstrInfo.cpp
4492 ↗(On Diff #161929)

const reference

4533 ↗(On Diff #161929)

I think value copies of MachineOperand don't behave like you would expect, so I usually avoid them

tpr updated this revision to Diff 162015.Aug 22 2018, 11:54 AM

V2: Addressed minor review comments.

tpr marked 2 inline comments as done.Aug 22 2018, 11:55 AM

Marek, I will correct the spelling of your name in the commit message when I land this. :-)

In D51098#1209699, @tpr wrote:

Marek, I will correct the spelling of your name in the commit message when I land this. :-)

Thanks. I was wondering who Marek Olsen was. I'd never heard of that guy. :)

nhaehnle added inline comments.Aug 23 2018, 1:16 AM
include/llvm/IR/IntrinsicsAMDGPU.td
809 ↗(On Diff #162015)

Can we make this consistent with the new (vector) buffer intrinsics?

lib/Target/AMDGPU/SIISelLowering.cpp
4947–4951 ↗(On Diff #162015)

clang-format?

tpr updated this revision to Diff 162239.EditedAug 23 2018, 11:34 AM

V3: i1 glc is now i32 cachepolicy for consistency with buffer and
tbuffer intrinsics, plus fixed formatting issue.

nhaehnle accepted this revision.Aug 24 2018, 4:08 AM

Thanks. I don't see a test that actually sets glc, please add one before committing.

Apart from that, LGTM.

This revision is now accepted and ready to land.Aug 24 2018, 4:08 AM
tpr added a comment.Aug 25 2018, 6:22 AM

Thanks, will do.

This revision was automatically updated to reflect the committed changes.