This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Introduce real and keep fake True16 instructions.
ClosedPublic

Authored by kosarev on Jul 24 2023, 4:00 AM.

Details

Summary

The existing fake True16 instructions using 32-bit VGPRs are supposed to
co-exist with real ones until all the necessary True16 functionality is
implemented and relevant tests are updated.

Diff Detail

Event Timeline

kosarev created this revision.Jul 24 2023, 4:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 24 2023, 4:00 AM
kosarev requested review of this revision.Jul 24 2023, 4:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 24 2023, 4:00 AM
foad added a comment.Jul 24 2023, 6:37 AM

Do we really need UseTrue16BitInsts? Can't we control this by enabling/disabling HasTrue16BitInsts, like Matt said on D156100?

Do we really need UseTrue16BitInsts? Can't we control this by enabling/disabling HasTrue16BitInsts, like Matt said on D156100?

We already have instructions and patterns that depend on HasTrue16BitInsts, so making it false by default would break existing GFX11 tests. UseTrue16BitInsts is for instructions and patterns that use real True16 instructions, so making it true by default would also break existing tests because the most of the True16 functionality is still missing. The plan is that these two flags should eventually become the same thing once the transition is done.

It seems like only one terminology, be it 'fake16' or 'not16' is required; can we just use one? I prefer fake16. Please add a detailed description of the 'legacy', 'fake16' and 'true16' versions of 16-bit instructions and add it to the commit message and perhaps a comment.

foad added a comment.Jul 28 2023, 6:18 AM

It seems like only one terminology, be it 'fake16' or 'not16' is required; can we just use one? I prefer fake16. Please add a detailed description of the 'legacy', 'fake16' and 'true16' versions of 16-bit instructions and add it to the commit message and perhaps a comment.

Agreed. I believe this patch is the first time the term "fake16" has been used anywhere, so we need a clear explanation of it.

kosarev updated this revision to Diff 545210.Jul 28 2023, 9:51 AM

Rename _not16 instructions and add explanations.

kosarev edited the summary of this revision. (Show Details)Jul 28 2023, 9:52 AM
kosarev edited the summary of this revision. (Show Details)
kosarev edited the summary of this revision. (Show Details)
kosarev updated this revision to Diff 545976.Aug 1 2023, 2:27 AM

Add AssemblerPredicates for the new subtarget feature to control
disassembling of True16 instructions as well.

kosarev edited the summary of this revision. (Show Details)Aug 1 2023, 2:28 AM
arsenm added inline comments.Aug 15 2023, 5:00 PM
llvm/lib/Target/AMDGPU/AMDGPU.td
1696–1698

Should get a comment here explaining "RealTrue" and "FakeTrue" are. I can forsee this overstaying its forseen lifetime...

llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.h
147–157

Can just submit this cleanup on its own

llvm/lib/Target/AMDGPU/SIInstructions.td
2704

I'm assuming the end goal is to bring back a more sensible naming?

rampitec added inline comments.Aug 15 2023, 5:02 PM
llvm/lib/Target/AMDGPU/AMDGPU.td
1696–1698

"FakeTrue" is truly really hilarious.

kosarev updated this revision to Diff 551119.Aug 17 2023, 6:45 AM

Add a comment for the predicates.

kosarev marked an inline comment as done.Aug 17 2023, 6:46 AM
kosarev added inline comments.
llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.h
147–157

Not a clean-up (though it can be). We use that for the FAKE16 tables, which don't exist prior this patch.

llvm/lib/Target/AMDGPU/SIInstructions.td
2704

Yes, fake16 entities are not supposed to stay. At some point a t16 variant of the pattern should be added and then the support for fake instructions should be removed after switching to using real True16 instructions by default.

Joe_Nash accepted this revision.Aug 22 2023, 7:20 AM

LGTM, but please wait for other reviewers to approve this take a look at other patches in the stack.

This revision is now accepted and ready to land.Aug 22 2023, 7:20 AM
arsenm accepted this revision.Aug 23 2023, 2:51 PM
This revision was landed with ongoing or failed builds.Sep 22 2023, 2:58 AM
This revision was automatically updated to reflect the committed changes.

@kosarev I'm not certain but I think this is causing breaks on EXPENSIVE_CHECKS builds: https://lab.llvm.org/buildbot/#/builders/42/builds/11707

Yep, I'm about to revert it.

Yep, I'm about to revert it.

6cb3866b1ce9d835402e414049478cea82427cf1

The failures were caused by a TableGen bug, now fixed in https://github.com/llvm/llvm-project/pull/67245. Preparing that fix took resolving the issue with long AMDGPUGenRegisterInfo.inc generation times, https://github.com/llvm/llvm-project/pull/67340, which in turn led to discovery of multiple failures on regression tests and fixing some of them in https://github.com/llvm/llvm-project/pull/67337.