Page MenuHomePhabricator

[AMDGPU][MC] Added PCK variants of image load/store instructions
ClosedPublic

Authored by dp on Mar 22 2018, 11:27 AM.

Details

Summary

See bug 36834: https://bugs.llvm.org/show_bug.cgi?id=36834

These instructions do not support 'd16' modifier.
Other than that documentation describes no differences from usual image load/store opcodes.

Diff Detail

Repository
rL LLVM

Event Timeline

dp created this revision.Mar 22 2018, 11:27 AM
nhaehnle added inline comments.Mar 23 2018, 2:45 AM
lib/Target/AMDGPU/MIMGInstructions.td
88 ↗(On Diff #139476)

What's actually the problem here? Why doesn't it work to directly have

defm _V1 : MIMG_NoSampler_Src_Helper_Helper <op, asm, VGPR_32, 1, 0, "">;

etc. in the multiclass below?

157 ↗(On Diff #139476)

Same here.

dp added inline comments.Mar 23 2018, 3:29 AM
lib/Target/AMDGPU/MIMGInstructions.td
88 ↗(On Diff #139476)

This is exactly what I did in the initial implementation.
I was surprised to see that in this case TableGen ignores "_V..." suffices specified in MIMG_PckNoSampler.
For example, instead of

IMAGE_LOAD_PCK_V1_V1
IMAGE_LOAD_PCK_V1_V2
IMAGE_LOAD_PCK_V1_V3
IMAGE_LOAD_PCK_V1_V4

IMAGE_LOAD_PCK_V2_V1
IMAGE_LOAD_PCK_V2_V2
IMAGE_LOAD_PCK_V2_V3
IMAGE_LOAD_PCK_V2_V4
...

TableGen generated the following names:

IMAGE_LOAD_PCK_V1
IMAGE_LOAD_PCK_V2
IMAGE_LOAD_PCK_V3
IMAGE_LOAD_PCK_V4

IMAGE_LOAD_PCK_V1
IMAGE_LOAD_PCK_V2
...

As a result I was getting a lot of duplicates and TableGen failed with an error

def "_V1" already defined in this multiclass.

I tried to use

defm NAME # "_V1"
...

but this did not help.

What am I missing here?

nhaehnle added inline comments.Mar 26 2018, 3:34 AM
lib/Target/AMDGPU/MIMGInstructions.td
88 ↗(On Diff #139476)

I see. Yeah, there are a bunch of really stupid action-at-a-distance problems with how TableGen resolves names. Have you tried also making the NAME explicit in the defs in MIMG_NoSampler_Src_Helper_Helper, like this:

def NAME # _V1 # suffix : ...;
def NAME # _V2 # suffix : ...;
def NAME # _V4 # suffix : ...;

It is not at all logical, but I think that could help, and in general if you explicitly refer to NAME at all levels of the multiclass nesting I believe it should always work as expected.

dp added inline comments.Mar 26 2018, 7:21 AM
lib/Target/AMDGPU/MIMGInstructions.td
88 ↗(On Diff #139476)

Thanks, this cured the problem! But I had to add "NAME" to MIMG_NoSampler_Src_Helper as well to make it work.

dp updated this revision to Diff 139786.Mar 26 2018, 7:23 AM

Corrected as suggested by Nicolai. Thanks!

dp added inline comments.Mar 26 2018, 7:27 AM
lib/Target/AMDGPU/MIMGInstructions.td
68 ↗(On Diff #139786)

Without this change MIMG_NoSampler generated names like this:

IMAGE_LOAD_MIP_V1anonymous_2455_V1

Instead of expected

IMAGE_LOAD_MIP_V1_V1
nhaehnle accepted this revision.Mar 27 2018, 1:03 AM

Thanks, LGTM.

lib/Target/AMDGPU/MIMGInstructions.td
68 ↗(On Diff #139786)

Yeah, good point. For what it's worth, I suspect that using defm "" instead of defm NAME would work as well, but either option is fine.

This revision is now accepted and ready to land.Mar 27 2018, 1:03 AM
This revision was automatically updated to reflect the committed changes.