This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][True16] Fix ISel for A16 Image Instructions
ClosedPublic

Authored by Joe_Nash on Aug 8 2023, 12:01 PM.

Details

Summary

The 16-bit VAddr arguments to A16 image instructions are packed into
legal VGPR_32 operands in AMDGPULegalizerInfo::legalizeImageIntrinsic on
all subtargets. With True16, we also need to pack if the number of VAddr is one
because VGPR_16 is not a legal argument to those Image instructions.

No change to emitted code intended on subtargets pre-GFX11, and none on GFX11
until True16 is active.

Diff Detail

Unit TestsFailed

Event Timeline

Joe_Nash created this revision.Aug 8 2023, 12:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2023, 12:01 PM
Joe_Nash requested review of this revision.Aug 8 2023, 12:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2023, 12:01 PM
kosarev added inline comments.Aug 9 2023, 5:09 AM
llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
5887

A comment explaining why it's important to include the case of NumVAddrs == 1 might be helpful.

kosarev added inline comments.Aug 9 2023, 5:32 AM
llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
5887

What are the cases where this condition is not met?

Joe_Nash updated this revision to Diff 549133.Aug 10 2023, 12:20 PM

The if statement is always true. Added comment

Joe_Nash marked 2 inline comments as done.Aug 10 2023, 12:22 PM
Joe_Nash added inline comments.
llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
5887

Good point. The condition is always true because all MIMG have at least one VAddr (afaik).

arsenm added inline comments.Aug 10 2023, 2:42 PM
llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
5890–5891

This deleted all the code by using a function it was already using?

foad accepted this revision.Aug 11 2023, 1:00 AM

LGTM. From reading SITargetLowering::lowerImage I think SelectionDAG already does it this way.

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp
5890–5891

The code below is just un-indented, not deleted.

llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-llvm.amdgcn.image.atomic.dim.a16.ll
684

Could do a pre-commit that just regenerates checks in these files, to avoid this kind of noise showing up in your patch.

This revision is now accepted and ready to land.Aug 11 2023, 1:00 AM
This revision was automatically updated to reflect the committed changes.
Joe_Nash marked 2 inline comments as done.