This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][AsmParser/Disassembler] Correct A16 and G16 handling
ClosedPublic

Authored by dstuttard on May 11 2021, 5:16 AM.

Details

Summary

A16 support for image instructions assembly/disassembly (gfx10) was missing

Also refactor MIMG op addr size calcs to common function

We'd got 3 places where the same operation was being done.

One test is now marked XFAIL until a related codegen patch is in place

Diff Detail

Event Timeline

dstuttard created this revision.May 11 2021, 5:16 AM
dstuttard requested review of this revision.May 11 2021, 5:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2021, 5:16 AM

This is a combination of the 2 previous patches - both asmparser and disassembler. I've also unified the code from SIInstrInfo.cpp into a single function since the code was duplicated 3 times before.

dstuttard updated this revision to Diff 344382.May 11 2021, 6:18 AM

Fixed sense of NoG16 argument passed in from asmparser and disassembler

dp added inline comments.May 11 2021, 8:06 AM
llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
159

I'd have renamed "NoG16" to "IsG16Supported" though this is a matter of taste.

169

Am I correct that A16 do affect gradient packing when G16 is not supported? In other words, A16 affects gradient packing on GFX9, right? (gfx9_shader_programming is silent about packing.)

As this is a badly documented feature, could you add a comment explaining how G16 and A16 affect gradient packing?

I compared the tests with your original patch and they are identical. Probably a few extra tests for _d/_cd opcodes with different combinations of A16 and G16 would be useful to have.

dstuttard added inline comments.May 11 2021, 8:54 AM
llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
159

Yes, I think you are right. I'll make that change.

169

Yes, that's right. If G16 isn't supported, A16 implies G16. (Yes, for gfx9 a16 makes gradients 16 bit too).

Yes, I'll add a comment to that effect.

You're correct, there aren't any new tests. I'll enhance those too.

dstuttard updated this revision to Diff 344732.May 12 2021, 2:32 AM

Made suggested changes

Increased test cases

dp added inline comments.May 13 2021, 4:22 AM
llvm/test/CodeGen/AMDGPU/llvm.amdgcn.image.sample.a16.dim.ll
4

I may be mistaken but this comment hints that you are going to submit this patch independently of
https://reviews.llvm.org/D102066. This won't work because many tests you added in the last update will fail - they depend on changes in MIMG_Sampler_AddrSizes. (Maybe there are more dependencies - I did not dig deeper.)

dstuttard updated this revision to Diff 345124.May 13 2021, 6:48 AM

Moved change from D102066 required for this change (which will be committed first)

dstuttard marked an inline comment as done.May 13 2021, 6:51 AM
dstuttard added inline comments.
llvm/test/CodeGen/AMDGPU/llvm.amdgcn.image.sample.a16.dim.ll
4

You are correct.
I've moved the change into this patch.

I've checked them independently this time and they both pass lit testing.

Not sure that trying to keep these as two patches was worthwhile.

dp added a comment.May 13 2021, 8:06 AM

Not sure that trying to keep these as two patches was worthwhile.

This definitely made review simpler for me, thanks!

dp accepted this revision.May 13 2021, 8:07 AM

LGTM

This revision is now accepted and ready to land.May 13 2021, 8:07 AM
This revision was automatically updated to reflect the committed changes.
dstuttard marked an inline comment as done.