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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.
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.
Add AssemblerPredicates for the new subtarget feature to control
disassembling of True16 instructions as well.
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? |
llvm/lib/Target/AMDGPU/AMDGPU.td | ||
---|---|---|
1696–1698 | "FakeTrue" is truly really hilarious. |
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. |
LGTM, but please wait for other reviewers to approve this take a look at other patches in the stack.
@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.
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.
Should get a comment here explaining "RealTrue" and "FakeTrue" are. I can forsee this overstaying its forseen lifetime...