This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][NFC] Add True16 operand definitions.
ClosedPublic

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

Diff Detail

Event Timeline

kosarev created this revision.Jul 24 2023, 4:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 24 2023, 4:05 AM
kosarev requested review of this revision.Jul 24 2023, 4:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 24 2023, 4:05 AM
Joe_Nash added inline comments.Jul 27 2023, 1:54 PM
llvm/lib/Target/AMDGPU/SIRegisterInfo.td
1247

Please add a comment saying when each of these 4 versions shall be used. I think the intention is:
VSrc_b16 - the current and temporary future default used case for VOP3
VSrcT_b16 - true16 VOP3
VSrcT_b16_Lo128 - true16 VOP1/2/C
VSrcFake16_b16_Lo128 - fake16 VOP1/2/C - the current and temporary future default used case for VOP1/2/C

llvm/lib/Target/AMDGPU/VOP2Instructions.td
381

Shall this be renamed to include fake16? I guess a distinct true16 version will be added at a later point.

406

Shall this be renamed to include fake16? I guess a distinct true16 version will be added at a later point.

kosarev updated this revision to Diff 545233.Jul 28 2023, 11:35 AM

Add comments.

kosarev marked an inline comment as done.Jul 28 2023, 11:36 AM
kosarev added inline comments.
llvm/lib/Target/AMDGPU/VOP2Instructions.td
381

Should be split into real and fake definitions later when there's immediate need for that.

406

Should be split into real and fake definitions later when there's immediate need for that.

Joe_Nash added inline comments.Jul 28 2023, 11:59 AM
llvm/lib/Target/AMDGPU/VOP2Instructions.td
406

But this *Is* the fake version, and when merged with the true16 version downstream both will exist. So I think it makes sense to rename immediately.

kosarev updated this revision to Diff 546593.Aug 2 2023, 1:48 PM

Update to add bits necessary for disassembling .h registers.

kosarev added inline comments.Aug 14 2023, 5:50 AM
llvm/lib/Target/AMDGPU/VOP2Instructions.td
406

when merged with the true16 version downstream both will exist.

Here upstream it will continue to be a fake instruction, which it is here, and downstream it will continue to be the real instruction, which it is there -- all until we decide to support both the versions.

There are hundreds of references to _t16 entities here in the mainline, none of which are real True16 things. Renaming them all right away would go against the whole idea of bringing in changes gradually.

Joe_Nash added inline comments.Aug 17 2023, 11:50 AM
llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
1474

Is it intentional that you're not calling decodeNonVGPRSrcOp here and instead duplicating parts of the function?

kosarev updated this revision to Diff 552319.Aug 22 2023, 5:08 AM

Update decodeSrcOp() to use decodeNonVGPRSrcOp().

llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
1474

Good catch. This is an oversight.

This revision is now accepted and ready to land.Aug 22 2023, 7:13 AM
This revision was automatically updated to reflect the committed changes.
nickdesaulniers added inline comments.
llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
265–318
[3892/4628] Building CXX object lib/Target/AMDGPU/Disassembler/CMakeFiles/LLVMAMDGPUDisassembler.dir/AMDGPUDisassembler.cpp.o
/android0/llvm-project/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:265:21: warning: unused function 'DecodeVGPR_16RegisterClass' [-Wunused-function]
static DecodeStatus DecodeVGPR_16RegisterClass(MCInst &Inst, unsigned Imm,
                    ^
/android0/llvm-project/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:278:1: warning: unused function 'DecodeVGPR_16_Lo128RegisterClass' [-Wunused-function]
DecodeVGPR_16_Lo128RegisterClass(MCInst &Inst, unsigned Imm, uint64_t /*Addr*/,
^
/android0/llvm-project/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:288:21: warning: unused function 'decodeOperand_VSrcT16_Lo128' [-Wunused-function]
static DecodeStatus decodeOperand_VSrcT16_Lo128(MCInst &Inst, unsigned Imm,
                    ^
/android0/llvm-project/llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:304:21: warning: unused function 'decodeOperand_VSrcT16' [-Wunused-function]
static DecodeStatus decodeOperand_VSrcT16(MCInst &Inst, unsigned Imm,
                    ^
kosarev added inline comments.Sep 25 2023, 9:13 AM
llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
265–318

The following patches down the chain will make these used; I'm about to submit them.