This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU/GlobalISel: Legalize unpacked d16 image operations
ClosedPublic

Authored by arsenm on Jan 15 2020, 11:50 AM.

Details

Reviewers
nhaehnle
kerbowa
Summary

On targets that don't have the normal packed f16 layout, handle these
during legalization. Directly modify the register types. We can infer
this was a d16 load based on the mem operand size during selection.

A16 operands should possibly be handled here as well, but don't worry
about that yet.

Diff Detail

Event Timeline

arsenm created this revision.Jan 15 2020, 11:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 15 2020, 11:50 AM

This seems largely reasonable to me, but I do think the observer notification needs to happen.

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
2776–2779

It might help the readability of legalizeIntrinsic to move this test there and pass the ImageDimIntr in?

2799

I think so?

arsenm marked an inline comment as done.Jan 27 2020, 9:07 AM
arsenm added inline comments.
llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
2799

legalizeIntrinsic doesn't have access to the observer, so I'll have to add that and audit the rest of these lowering at some point

arsenm updated this revision to Diff 240614.Jan 27 2020, 9:24 AM

Move ImageDimIntrinsicInfo check

nhaehnle accepted this revision.Jan 30 2020, 1:49 AM

Thanks, LGTM modulo the observer question -- I tend to think it's necessary, but I'll defer to your judgment.

This revision is now accepted and ready to land.Jan 30 2020, 1:49 AM

Thanks, LGTM modulo the observer question -- I tend to think it's necessary, but I'll defer to your judgment.

Thanks, LGTM modulo the observer question -- I tend to think it's necessary, but I'll defer to your judgment.

I added the needed observer argument in c5fffa4da35f0fcc89b5ea88cc1bc60bc475a18e, so I've fixed this.