Page MenuHomePhabricator

AMDGPU: Add a16 feature to gfx10
AbandonedPublic

Authored by arsenm on Jan 17 2020, 9:01 AM.

Details

Summary

It seems like this was accidentally dropped, breaking all a16 image
intrinsics for gfx10. This fixes failing/asserting during
lowering. With the feature added, the selected instruction fails the
verifier. Guess at how to fix this, which seems to have confused the
dword and component counts.

Duplicate the test and generate the checks since I know very little
about how these are really supposed to operate.

Diff Detail

Event Timeline

arsenm created this revision.Jan 17 2020, 9:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 17 2020, 9:01 AM
arsenm updated this revision to Diff 238812.Jan 17 2020, 9:58 AM
arsenm edited the summary of this revision. (Show Details)
arsenm added a reviewer: rtaylor.
rampitec added inline comments.Jan 17 2020, 10:38 AM
llvm/lib/Target/AMDGPU/AMDGPU.td
851

Could you add it right to the FeatureGFX10 maybe?

sebastian-ne marked an inline comment as done.Mon, Jan 27, 6:50 AM
sebastian-ne added a subscriber: sebastian-ne.

The added CodeGen/AMDGPU/llvm.amdgcn.image.a16.dim.ll fails for gfx10, but a16 does not work anyway at the moment.

llvm/lib/Target/AMDGPU/SIISelLowering.cpp
5628

Are you sure that’s right?
The test CodeGen/AMDGPU/llvm.amdgcn.image.nsa.ll fails for me with this change.

arsenm marked an inline comment as done.Mon, Jan 27, 6:53 AM
arsenm added inline comments.
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
5628

No, it's probably wrong

sebastian-ne added inline comments.Wed, Jan 29, 6:02 AM
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
3679

I think this should be the following because the extra arguments like bias are still f32 and should not be divided by two:

unsigned AddrComponents =
                     (BaseOpcode->Gradients ? Dim->NumGradients : 0) +
                     (BaseOpcode->Coordinates ? Dim->NumCoords : 0) +
                     (BaseOpcode->LodOrClampOrMip ? 1 : 0);

unsigned AddrWords = BaseOpcode->NumExtraArgs +
                     (IsA16 ? (AddrComponents + 1) / 2 : AddrComponents);
arsenm marked an inline comment as done.Wed, Jan 29, 7:08 AM
arsenm added inline comments.
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
3679

Feel free to commandeer, I'm probably not going to get back to fixing the DAG version anytime soon

arsenm abandoned this revision.Tue, Feb 4, 2:02 PM