[AMDGPU][True16][CodeGen] fix moveToVALU with proper subreg access in true16

[broxigarchen]

There are V2S copies between vpgr16 and spgr32 in true16 mode. This is caused by vgpr16 and sgpr32 both selectable by 16bit src in ISel.

When a V2S copy and its useMI are lowered to VALU, this patch check

1. If the generated new VALU is used by a true16 inst. Add subreg access if necessary.
2. Legalize the V2S copy by replacing it to subreg_to_reg

an example MIR looks like:

%2:sgpr_32 = COPY %1:vgpr_16
%3:sgpr_32 = S_OR_B32 %2:sgpr_32, ...
%4:vgpr_16 = V_ADD_F16_t16 %3:sgpr_32, ...

currently lowered to

%2:vgpr_32 = COPY %1:vgpr_16
%3:vgpr_32 = V_OR_B32 %2:vgpr_32, ...
%4:vgpr_16 = V_ADD_F16_t16 %3:vgpr_32, ...

after this patch

%2:vgpr_32 = SUBREG_TO_REG 0, %1:vgpr_16, lo16
%3:vgpr_32 = V_OR_B32 %2:vgpr_32, ...
%4:vgpr_16 = V_ADD_F16_t16 %3.lo16:vgpr_32, ...

[llvmbot]

@llvm/pr-subscribers-backend-amdgpu

Author: Brox Chen (broxigarchen)

Changes

When a SGPR copy is lowered to a VALU, check if the new VALU instruction is used by a true16 instructions. Add subreg access if necessary.

---

Full diff: https://github.com/llvm/llvm-project/pull/132089.diff

2 Files Affected:

• (modified) llvm/lib/Target/AMDGPU/SIInstrInfo.cpp (+16)
• (modified) llvm/test/CodeGen/AMDGPU/fneg-combines.f16.ll (+1-1)

diff --git a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
index fb791c8342282..5f2bd507d1767 100644
--- a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
+++ b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
@@ -7835,6 +7835,22 @@ void SIInstrInfo::moveToVALUImpl(SIInstrWorklist &Worklist,
     assert(NewDstRC);
     NewDstReg = MRI.createVirtualRegister(NewDstRC);
     MRI.replaceRegWith(DstReg, NewDstReg);
+
+    // Check useMI of NewInstr. If used by a true16 instruction,
+    // add a lo16 subreg access if size mismatched
+    if (ST.useRealTrue16Insts() && NewDstRC == &AMDGPU::VGPR_32RegClass) {
+      for (MachineRegisterInfo::use_iterator I = MRI.use_begin(NewDstReg),
+                                             E = MRI.use_end();
+           I != E; ++I) {
+        MachineInstr &UseMI = *I->getParent();
+        unsigned UseMIOpcode = UseMI.getOpcode();
+        if (AMDGPU::isTrue16Inst(UseMIOpcode) &&
+            (16 ==
+             RI.getRegSizeInBits(*getOpRegClass(UseMI, I.getOperandNo())))) {
+          I->setSubReg(AMDGPU::lo16);
+        }
+      }
+    }
   }
   fixImplicitOperands(*NewInstr);
   // Legalize the operands
diff --git a/llvm/test/CodeGen/AMDGPU/fneg-combines.f16.ll b/llvm/test/CodeGen/AMDGPU/fneg-combines.f16.ll
index 5ea39997938ad..4f6b334ec0819 100644
--- a/llvm/test/CodeGen/AMDGPU/fneg-combines.f16.ll
+++ b/llvm/test/CodeGen/AMDGPU/fneg-combines.f16.ll
@@ -699,7 +699,7 @@ define amdgpu_ps half @fneg_fadd_0_f16(half inreg %tmp2, half inreg %tmp6, <4 x
 ; GFX11-SAFE-TRUE16-NEXT:    v_cmp_ngt_f16_e32 vcc_lo, s0, v0.l
 ; GFX11-SAFE-TRUE16-NEXT:    s_delay_alu instid0(VALU_DEP_2) | instskip(NEXT) | instid1(VALU_DEP_1)
 ; GFX11-SAFE-TRUE16-NEXT:    v_xor_b32_e32 v0, 0x8000, v1
-; GFX11-SAFE-TRUE16-NEXT:    v_cndmask_b16 v0.l, v0/*Invalid register, operand has 'VS_16' register class*/, s0, vcc_lo
+; GFX11-SAFE-TRUE16-NEXT:    v_cndmask_b16 v0.l, v0.l, s0, vcc_lo
 ; GFX11-SAFE-TRUE16-NEXT:    s_delay_alu instid0(VALU_DEP_1)
 ; GFX11-SAFE-TRUE16-NEXT:    v_cmp_nlt_f16_e32 vcc_lo, 0, v0.l
 ; GFX11-SAFE-TRUE16-NEXT:    v_cndmask_b16 v0.l, 0x7e00, 0, vcc_lo

[broxigarchen]

We probably should also fix the bad copy %2:vgpr_32 = COPY %1:vgpr_16 and select t16 VALU directly

changing this PR to draft for now

[broxigarchen]

It seems it takes more work to select t16 VALU during moveToVALU. Can be a seperate patch. Currently just fix the bad copy %2:vgpr_32 = COPY %1:vgpr_16.

Close this patch #131859 and proposed this as an alternative fix

[broxigarchen]

rebased

[broxigarchen]

Gentle ping!

[Sisyph]

There was a comment on the original PR #131859 from @arsenm that 'you should be able to get this correct from the start'. I don't think we can get it right from the start, because the core issue is putting both 16 and 32 bit values into SGPR32.

So this patch and #131859 are different ways of fixing that up. This patch is better in one way because it attaches proper subregisters, where the other one doesn't. But this patch adds more special purpose code.

I see we don't have exiting uses of SUBREG_TO_REG in the AMDGPU backend. Where is that lowered to a target instruction? Do we expect passes seeing it to understand it? If so, this patch seems ok to me.

[broxigarchen]

There was a comment on the original PR #131859 from @arsenm that 'you should be able to get this correct from the start'. I don't think we can get it right from the start, because the core issue is putting both 16 and 32 bit values into SGPR32.

So this patch and #131859 are different ways of fixing that up. This patch is better in one way because it attaches proper subregisters, where the other one doesn't. But this patch adds more special purpose code.

I see we don't have exiting uses of SUBREG_TO_REG in the AMDGPU backend. Where is that lowered to a target instruction? Do we expect passes seeing it to understand it? If so, this patch seems ok to me.

Hi Joe. SUBREG_TO_REG is lowered to copy in a codegen pass https://github.com/llvm/llvm-project/blob/main/llvm/lib/CodeGen/ExpandPostRAPseudos.cpp. Also it's understandable by other codegen pass such as RA and coalescer

[Sisyph]

LGTM

[broxigarchen]

rebased