This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Add llvm.amdgcn.interp.mov intrinsic
ClosedPublic

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

Diff Detail

Repository
rL LLVM

Event Timeline

tstellarAMD retitled this revision from to AMDGPU: Add llvm.amdgcn.interp.mov intrinsic.
tstellarAMD updated this object.
tstellarAMD added reviewers: arsenm, nhaehnle.
tstellarAMD added a subscriber: llvm-commits.
arsenm added inline comments.Nov 15 2016, 6:22 PM
include/llvm/IR/IntrinsicsAMDGPU.td
481 ↗(On Diff #78119)

Shouldn't this be IntrReadMem? The doc says it reads parameters in LDS

test/CodeGen/AMDGPU/llvm.amdgcn.interp.ll
18 ↗(On Diff #78119)

Do the source modifies work? There should be a test using them

include/llvm/IR/IntrinsicsAMDGPU.td
481 ↗(On Diff #78119)

I think technically, this should be inaccessiblememonly, because but there is no intrinsic attribute for this yet.

test/CodeGen/AMDGPU/llvm.amdgcn.interp.ll
18 ↗(On Diff #78119)

There are no source modifiers for the VINTRP instructions.

nhaehnle edited edge metadata.Nov 18 2016, 8:18 AM

LGTM

include/llvm/IR/IntrinsicsAMDGPU.td
481 ↗(On Diff #78119)

And that region of LDS is constant anyway, so IntrNoMem is fine.

arsenm added inline comments.Nov 18 2016, 10:12 AM
include/llvm/IR/IntrinsicsAMDGPU.td
481 ↗(On Diff #78119)

Is that possible? I thought the runtime couldn't ever initialize LDS, so something in the shader has to set it in the first place

nhaehnle added inline comments.Nov 18 2016, 12:01 PM
include/llvm/IR/IntrinsicsAMDGPU.td
481 ↗(On Diff #78119)

The LDS for these instructions is initialized by Primitive Assembly (or possibly the Scan Converter, it's unclear to me where the boundary between those hardware blocks is precisely).

mareko added a subscriber: mareko.Nov 21 2016, 5:04 PM
mareko added inline comments.
include/llvm/IR/IntrinsicsAMDGPU.td
481 ↗(On Diff #78119)

LDS is initialized by the SPI block before the pixel shader. ReadNone is fine, because nothing can modify it.

arsenm added inline comments.Nov 21 2016, 5:07 PM
include/llvm/IR/IntrinsicsAMDGPU.td
481 ↗(On Diff #78119)

I don't think it can really prevent writes? I'm betting an out of bounds access will clobber it. BTW D26485 adds the property

mareko added inline comments.Nov 21 2016, 5:32 PM
include/llvm/IR/IntrinsicsAMDGPU.td
481 ↗(On Diff #78119)

I think you'd have to modify m0 to before writing to it, which can't be done from LLVM IR. Anyway, the same could be said about any other memory. We want v_interp to be 100% movable without restrictions. We want the same for the vast majority of loads and also most stores, because stores with side effects are very rare in graphics, and are usually limited to LDS in compute shaders. I've seen plenty of buffer and image stores used by games, but I've yet to see those with any side effects.

This revision was automatically updated to reflect the committed changes.