This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] New tbuffer intrinsics
ClosedPublic

Authored by tpr on Jul 6 2018, 8:07 AM.

Details

Summary

This commit adds new intrinsics

llvm.amdgcn.tbuffer2.load
llvm.amdgcn.tbuffer2.store

with combined operands for format (dfmt+nfmt) and cachepolicy (glc+slc).

The AMDISD::TBUFFER_* SD nodes have changed correspondingly.

The obsolescent llvm.amdgcn.tbuffer.* and llvm.SI.tbuffer.store
intrinsics continue to work.

Change-Id: If22ad77e349fac3a5d2f72dda53c010377d470d4

Diff Detail

Event Timeline

tpr created this revision.Jul 6 2018, 8:07 AM
tpr added a reviewer: Restricted Project.Jul 16 2018, 7:12 AM
dstuttard accepted this revision as: dstuttard.Jul 23 2018, 7:39 AM

LGTM - but you might want further reviews from others not so involved in implementation.

This revision is now accepted and ready to land.Jul 23 2018, 7:39 AM
tpr updated this revision to Diff 157568.Jul 26 2018, 1:35 PM

V2: Separate raw and struct intrinsics.

tpr updated this revision to Diff 157655.Jul 27 2018, 1:45 AM

V3: Moved extract_glc and extract_slc defs to a more sensible place.

arsenm added inline comments.Jul 27 2018, 1:57 AM
test/CodeGen/AMDGPU/llvm.amdgcn.tbuffer.store.ll
73–80

Why the change in argument values?

tpr added inline comments.Jul 28 2018, 2:00 AM
test/CodeGen/AMDGPU/llvm.amdgcn.tbuffer.store.ll
73–80

16 is an illegal dfmt (it is only 4 bits), so the test was invalid.

nhaehnle accepted this revision as: nhaehnle.Jul 30 2018, 4:32 AM

Thanks for doing this.

It's not entirely clear to me from how the web diff is shown: you're leaving the tests for the old intrinsics in place, right?

If so, the change looks good to me.

tpr updated this revision to Diff 157996.Jul 30 2018, 10:31 AM

V4: Rebased on D49995.

tpr added a comment.Jul 30 2018, 10:32 AM

Yes, I have left the old intrinsics and their tests in place.

tpr updated this revision to Diff 159199.Aug 5 2018, 2:35 AM

V5: Only two separate offset args instead of three.
V6: Pseudo- and real instructions have joint format operand.

tpr updated this revision to Diff 159271.Aug 6 2018, 4:14 AM

V7: Restored optionality of dfmt and nfmt in assembler.

Some smaller nitpicks, but mostly looks good to me!

lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
3556 ↗(On Diff #159271)

Maybe an out-of-bounds check here?

4800 ↗(On Diff #159271)

Don't we still need the "nfmt" line here as well, in case the order is reversed? I see you've added a test for that, but I don't actually understand how that test is passing...

lib/Target/AMDGPU/SIISelLowering.cpp
5569

Just SDValue() should do the trick.

5582

Can just be !N0, like below.

5594

I think we should be able to just say return {N0, SDValue(C1, 0)}; here.

tpr added inline comments.Aug 17 2018, 2:43 AM
lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
4800 ↗(On Diff #159271)

The name field in AMDGPUOptionalOperandTable is only used if it's one of a bunch of optional operands parsed by the same function, e.g. parseIntWithPrefix. Because AMDGPUOperand::ImmTyFORMAT is parsed by its own function parseDfmtNfmt, it is up to that function to determine whether we do in fact have a dfmt+nfmt operand (starting with either "dfmt" or "nfmt"), and say "no match" if we don't.

tpr updated this revision to Diff 161193.Aug 17 2018, 2:54 AM

V8: Addressed minor review comments.

This revision was automatically updated to reflect the committed changes.