This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][True16] Support emitting copies between different register sizes.
ClosedPublic

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

Diff Detail

Event Timeline

kosarev created this revision.Jul 24 2023, 4:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 24 2023, 4:12 AM
kosarev requested review of this revision.Jul 24 2023, 4:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 24 2023, 4:12 AM
arsenm added inline comments.Jul 24 2023, 6:15 AM
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
742–746

Is this actually reachable? I forget how exactly we ended up with this partial 16-bit register thing

rampitec added inline comments.Jul 24 2023, 10:52 AM
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
743

Are these tabs?

kosarev added inline comments.Jul 24 2023, 11:44 AM
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
742–746

I'm going to try to understand more about what's going on here, but maybe @Joe_Nash already knows the answer as he was working on that bit.

743

Nope, just re-indendting with proper spaces.

Joe_Nash added inline comments.Jul 27 2023, 1:22 PM
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
742–746

It is covered by lo16-32bit-physreg-copy.mir.

In theory it is reachable on any target with 16 bit instructions (excluding GF11 with True 16 bit instructions). This code converts
COPY %1:vgpr_32 = %2:vgpr_lo16 or COPY %1:vgpr_lo16 = %2:vgpr_32
into v_mov_b32 on those targets.
On GFX11 it converts to v_mov_b16

Now as to whether the code sequence in that mir test (or any use of a VGPR_LO16 register) is produced in any legitimate shader, I don't think so. That probably needs to be verified empirically on a test corpus, but then this code and I think VGPR_LO16/ VGPR_HI16 can be removed.

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

Support generating differently-sized register transfers.

They are called copies, not transfers. How about: "Support emitting copies between different register sizes"?

kosarev updated this revision to Diff 546005.Aug 1 2023, 4:56 AM

Change the commit title as suggested and remove the unused code handling
the non-True16 case along with the test covering it.

kosarev retitled this revision from [AMDGPU][True16] Support generating differently-sized register transfers. to [AMDGPU][True16] Support emitting copies between different register sizes..Aug 1 2023, 4:57 AM

Change the commit title as suggested and remove the unused code handling
the non-True16 case along with the test covering it.

Can you please do this case removal as a separate patch?

kosarev updated this revision to Diff 546788.Aug 3 2023, 3:41 AM

Restore the non-True16 case branch to then remove it with a separate patch.

This revision is now accepted and ready to land.Aug 22 2023, 6:31 AM