Handling ADD|SUB U64 decomposed Pseudos not getting lowered to SDWA form

[yassingh]

Summary

This patch fixes some of the V_ADD/SUB_U64_PSEUDO not getting converted to their sdwa form.
We still get below patterns in generated code:
v_and_b32_e32 v0, 0xff, v0
v_add_co_u32_e32 v0, vcc, v1, v0
v_addc_co_u32_e64 v1, s[0:1], 0, 0, vcc

and,
v_and_b32_e32 v2, 0xff, v2
v_add_co_u32_e32 v0, vcc, v0, v2
v_addc_co_u32_e32 v1, vcc, 0, v1, vcc

1st and 2nd instructions of both above examples should have been folded into sdwa add with BYTE_0 src operand.

The reason being the pseudo instruction is broken down into VOP3 instruction pair of V_ADD_CO_U32_e64 and V_ADDC_U32_e64.
The sdwa pass attempts lowering them to their VOP2 form before converting them into sdwa instructions. However V_ADDC_U32_e64
cannot be shrunk to it's VOP2 form if it has non-reg src1 operand.
This change attempts to fix that problem by only shrinking V_ADD_CO_U32_e64 instruction.

[rampitec]

llvm/lib/Target/AMDGPU/SIPeepholeSDWA.cpp

871

This can be inline literal and still useful I suppose.

[arsenm]

llvm/lib/Target/AMDGPU/SIPeepholeSDWA.cpp

924–927

This is increasing the instruction size (and most likely the code size). This only makes sense to do if we know the fold into the operand can happen. This should perform those legality checks and make the full transform

[yassingh]

llvm/lib/Target/AMDGPU/SIPeepholeSDWA.cpp

871

Sorry I don't understand, are you suggesting moving this condition to a string literal?

924–927

Yes. There are 2 possible scenarios, if only the src1 operand is immediate then the 'mov' instruction will be folded. If both operands are immediates then 'MOV' will be part of generated assembly. Function 1 and 2 in the both test-files depict these scenarios.

I can add the legality check for this fold happening but then some of the instructions will be missed?

[foad]

Can you precommit the tests and rebase so that we can see the diffs?

llvm/test/CodeGen/AMDGPU/v_add_u64_pseudo_sdwa.ll

9–10

This is silly. You have added a mov instruction to shrink the addc instruction for no reason, because it is not actually converted to sdwa form.

[yassingh]

Pre-committed the tests and rebased the diff

[yassingh]

llvm/test/CodeGen/AMDGPU/v_add_u64_pseudo_sdwa.ll

9–10

Yes you are right but the pass right now only attempts to convert to sdwa when both instructions (V_ADD_CO and V_ADDC_CO) are shrinkable to their sdwa formed so I went this way.
I can explore only shrinking the v_add_co instruction and avoid inserting that mov instruction?

[rampitec]

llvm/lib/Target/AMDGPU/SIPeepholeSDWA.cpp

871

I mean bailing on non-register operand limits the pass. The src1 operand can be inline literal and still convertible to sdwa form.

[yassingh]

Only shrink V_ADD_CO_U32_e32 instruction if V_ADDC_CO_U32_e32 is not shrinkable. Hence avoiding introducing redundant copy instruction as suggested by @arsenm and @foad

[yassingh]

llvm/lib/Target/AMDGPU/SIPeepholeSDWA.cpp

871

Earlier we were checking if the instruction is not shrinkable don't proceed. Now we are also checking whether the reason for not shrinking is src1 being an immediate operand.
Hence we are covering src1 being an inline literal(correct me if I'm wrongly assuming that inline literal means immediate operand)

[arsenm]

llvm/lib/Target/AMDGPU/SIPeepholeSDWA.cpp

871

Inline immediate ares the ones that are free to encode in vsrc0, or in all vsrc operands for VOP3, as opposed to other literals that require materialization in a register. See TII::isInlineConstant

[rampitec]

llvm/lib/Target/AMDGPU/SIPeepholeSDWA.cpp

871

If an immediate is an inline literal instruction is still shrinkable. For example if it is 0 or 1 it is shrinkable, if it is 100 it is not.

[cdevadas]

llvm/lib/Target/AMDGPU/SIPeepholeSDWA.cpp

871

Need a comment here for allowing non-register vsrc1 cases.

[yassingh]

Since only v_add_co_u32_e32 can be converted to SDWA form there is no need to try convert v_addc_co_u32_e64 to it's VOP2 form. This will allow us to get rid of all the checks we were performing on v_addc_co_u32. The shrinking of this instr will be taken care by si-shrink-instructions.

[foad]

llvm/tmp.mir

71 ↗

(On Diff #474487)

This can be much simpler. Typically you only need name: and tracksRegLiveness: and body:.

If you used llc to generate this then you should try adding -simplify-mir.

[yassingh]

llvm/tmp.mir

71 ↗

(On Diff #474487)

Thanks for pointing out, accidently added this file to review. Removing now

[foad]

Looks good overall.

llvm/lib/Target/AMDGPU/SIPeepholeSDWA.cpp

902

"output", "its" (no apostrophe), full stop at end of sentence.

904

Doesn't this need to be VCC_LO for wave32? Please add a test for that. You can use SIRegisterInfo::getVCC() to get the appropriate reg for the wave size.

[foad]

This change attempts to fix that problem by introducing a register operand in place of immediate src1 operand.

You're not doing that any more, so the description needs updating.

[arsenm]

llvm/lib/Target/AMDGPU/SIPeepholeSDWA.cpp

853

Only converting the low half to VOP2 is somewhat surprising given the name, but I guess this makes sense

[yassingh]

Addressed review comments, updated summary.

[yassingh]

llvm/lib/Target/AMDGPU/SIPeepholeSDWA.cpp

904

Updated to TRI->getVCC(). However I am not able to add test for VCC_LO. Tried compiling for gfx1010, wavefrontsize=32 but SIInstrInfo::canShrink returns false for V_ADD_CO_U32_e64 hence the pass does not attempt converting it to sdwa form.

Responsible condition in SIInstrInfo::canShrink() => if (!hasVALU32BitEncoding(MI.getOpcode())) return false;

[foad]

llvm/lib/Target/AMDGPU/SIPeepholeSDWA.cpp

904

Oh you're right, GFX10/11 V_ADD_CO_U32 does not have an e32 or sdwa form. Sorry.

[arsenm]

LGTM

llvm/lib/Target/AMDGPU/SIPeepholeSDWA.cpp

for (MachineInstr &MI : MBB) {
     if (auto Operand = matchSDWAOperand(MI)) {
       LLVM_DEBUG(dbgs() << "Match: " << MI << "To: " << *Operand << '\n');
       SDWAOperands[&MI] = std::move(Operand);
       ++NumSDWAPatternsFound;
     }
   }
 }
 
-// Convert the V_ADDC_U32_e64 into V_ADDC_U32_e32, and
-// V_ADD_CO_U32_e64 into V_ADD_CO_U32_e32. This allows isConvertibleToSDWA
-// to perform its transformation on V_ADD_CO_U32_e32 into V_ADD_CO_U32_sdwa.
+// Convert the V_ADD_CO_U32_e64 into V_ADD_CO_U32_e32. This allows
+// isConvertibleToSDWA to perform its transformation on V_ADD_CO_U32_e32 into
+// V_ADD_CO_U32_sdwa.
 //
 // We are transforming from a VOP3 into a VOP2 form of the instruction.
 //   %19:vgpr_32 = V_AND_B32_e32 255,
 //       killed %16:vgpr_32, implicit $exec
 //   %47:vgpr_32, %49:sreg_64_xexec = V_ADD_CO_U32_e64
 //       %26.sub0:vreg_64, %19:vgpr_32, implicit $exec
 //  %48:vgpr_32, dead %50:sreg_64_xexec = V_ADDC_U32_e64
 //       %26.sub1:vreg_64, %54:vgpr_32, killed %49:sreg_64_xexec, implicit $exec
 //
 // becomes
 //   %47:vgpr_32 = V_ADD_CO_U32_sdwa
 //       0, %26.sub0:vreg_64, 0, killed %16:vgpr_32, 0, 6, 0, 6, 0,
 //       implicit-def $vcc, implicit $exec
-//  %48:vgpr_32 = V_ADDC_U32_e32
-//       0, %26.sub1:vreg_64, implicit-def $vcc, implicit $vcc, implicit $exec
+//  %48:vgpr_32, dead %50:sreg_64_xexec = V_ADDC_U32_e64
+//       %26.sub1:vreg_64, %54:vgpr_32, killed $vcc, implicit $exec
 void SIPeepholeSDWA::pseudoOpConvertToVOP2(MachineInstr &MI,
                                            const GCNSubtarget &ST) const {
   int Opc = MI.getOpcode();
   assert((Opc == AMDGPU::V_ADD_CO_U32_e64 || Opc == AMDGPU::V_SUB_CO_U32_e64) &&
          "Currently only handles V_ADD_CO_U32_e64 or V_SUB_CO_U32_e64");
 
   // Can the candidate MI be shrunk?
   if (!TII->canShrink(MI, *MRI))
     return;
   Opc = AMDGPU::getVOPe32(Opc);
   // Find the related ADD instruction.
   const MachineOperand *Sdst = TII->getNamedOperand(MI, AMDGPU::OpName::sdst);
   if (!Sdst)
     return;
   MachineOperand *NextOp = findSingleRegUse(Sdst, MRI);
   if (!NextOp)
     return;
   MachineInstr &MISucc = *NextOp->getParent();
-  // Can the successor be shrunk?
-  if (!TII->canShrink(MISucc, *MRI))
-    return;
-  int SuccOpc = AMDGPU::getVOPe32(MISucc.getOpcode());
+
   // Make sure the carry in/out are subsequently unused.
   MachineOperand *CarryIn = TII->getNamedOperand(MISucc, AMDGPU::OpName::src2);
   if (!CarryIn)
     return;
   MachineOperand *CarryOut = TII->getNamedOperand(MISucc, AMDGPU::OpName::sdst);
   if (!CarryOut)
     return;
   if (!MRI->hasOneUse(CarryIn->getReg()) || !MRI->use_empty(CarryOut->getReg()))
     return;
   // Make sure VCC or its subregs are dead before MI.
   MachineBasicBlock &MBB = *MI.getParent();
   auto Liveness = MBB.computeRegisterLiveness(TRI, AMDGPU::VCC, MI, 25);
   if (Liveness != MachineBasicBlock::LQR_Dead)
     return;
   // Check if VCC is referenced in range of (MI,MISucc].
   for (auto I = std::next(MI.getIterator()), E = MISucc.getIterator();
        I != E; ++I) {
     if (I->modifiesRegister(AMDGPU::VCC, TRI))
       return;
   }
 
-  // Make the two new e32 instruction variants.
   // Replace MI with V_{SUB|ADD}_I32_e32
   BuildMI(MBB, MI, MI.getDebugLoc(), TII->get(Opc))
     .add(*TII->getNamedOperand(MI, AMDGPU::OpName::vdst))
     .add(*TII->getNamedOperand(MI, AMDGPU::OpName::src0))
     .add(*TII->getNamedOperand(MI, AMDGPU::OpName::src1))
     .setMIFlags(MI.getFlags());
 
   MI.eraseFromParent();
 
-  // Replace MISucc with V_{SUBB|ADDC}_U32_e32
-  BuildMI(MBB, MISucc, MISucc.getDebugLoc(), TII->get(SuccOpc))
-    .add(*TII->getNamedOperand(MISucc, AMDGPU::OpName::vdst))
-    .add(*TII->getNamedOperand(MISucc, AMDGPU::OpName::src0))
-    .add(*TII->getNamedOperand(MISucc, AMDGPU::OpName::src1))
-    .setMIFlags(MISucc.getFlags());
+  // Since the carry outpur of MI is now VCC, update it's use in MISucc
 
-  MISucc.eraseFromParent();
+  MISucc.substituteRegister(CarryIn->getReg(), AMDGPU::VCC, 0, *TRI);
 }
 
 bool SIPeepholeSDWA::isConvertibleToSDWA(MachineInstr &MI,
                                          const GCNSubtarget &ST) const {
   // Check if this is already an SDWA instruction
   unsigned Opc = MI.getOpcode();
   if (TII->isSDWA(Opc))
     return true;
 
   // Check if this instruction has opcode that supports SDWA
   if (AMDGPU::getSDWAOp(Opc) == -1)
     Opc = AMDGPU::getVOPe32(Opc);
 
   if (AMDGPU::getSDWAOp(Opc) == -1)
     return false;
 
   if (!ST.hasSDWAOmod() && TII->hasModifiersSet(MI, AMDGPU::OpName::omod))
     return false;
 
   if (TII->isVOPC(Opc)) {
     if (!ST.hasSDWASdst()) {
       const MachineOperand *SDst = TII->getNamedOperand(MI, AMDGPU::OpName::sdst);
       if (SDst && (SDst->getReg() != AMDGPU::VCC &&
                    SDst->getReg() != AMDGPU::VCC_LO))
         return false;
     }
 
     if (!ST.hasSDWAOutModsVOPC() &&
         (TII->hasModifiersSet(MI, AMDGPU::OpName::clamp) ||
          TII->hasModifiersSet(MI, AMDGPU::OpName::omod)))
       return false;

llvm/test/CodeGen/AMDGPU/sdwa-ops.mir

 # RUN: llc -march=amdgcn -mcpu=gfx900 -verify-machineinstrs -run-pass=si-peephole-sdwa -o - %s | FileCheck -check-prefix=GFX9 %s
 # RUN: llc -march=amdgcn -mcpu=fiji -verify-machineinstrs -run-pass=si-peephole-sdwa -o - %s | FileCheck -check-prefix=GFX9 %s
 
 # test for 3 consecutive _sdwa's
 # GFX9-LABEL: name:            test1_add_co_sdwa
 # GFX9: = nsw V_ADD_CO_U32_sdwa
-# GFX9-NEXT: = nuw V_ADDC_U32_e32
+# GFX9-NEXT: = nuw V_ADDC_U32_e64
 # GFX9: V_ADD_CO_U32_sdwa
-# GFX9-NEXT: V_ADDC_U32_e32
+# GFX9-NEXT: V_ADDC_U32_e64
 # GFX9: V_ADD_CO_U32_sdwa
-# GFX9-NEXT: V_ADDC_U32_e32
+# GFX9-NEXT: V_ADDC_U32_e64
 ---
 name:            test1_add_co_sdwa
 tracksRegLiveness: true
 registers:
   - { id: 0, class: vgpr_32, preferred-register: '' }
 liveins:
   - { reg: '$vgpr0', virtual-reg: '%0' }
   - { reg: '$sgpr0_sgpr1', virtual-reg: '%1' }
bb.0:
     %172:vreg_64 = REG_SEQUENCE %173, %subreg.sub0, %174, %subreg.sub1
     GLOBAL_STORE_DWORDX2_SADDR %30.sub0, %172, %1, 0, 0, implicit $exec, implicit $exec :: (store (s64))
 
 ...
 
 # test for VCC interference on sdwa, should generate 1 xform only
 # GFX9-LABEL: name:            test2_add_co_sdwa
 # GFX9: V_ADD_CO_U32_sdwa
-# GFX9: V_ADDC_U32_e32
+# GFX9: V_ADDC_U32_e64
 # GFX9-NOT: V_ADD_CO_U32_sdwa
 # GFX9-NOT: V_ADDC_U32_e32
 ---
 name:            test2_add_co_sdwa
 tracksRegLiveness: true
 registers:
   - { id: 0, class: vgpr_32, preferred-register: '' }
 liveins:
bb.0:
     GLOBAL_STORE_DWORDX2_SADDR %30.sub0, %62, %1, 0, 0, implicit $exec, implicit $exec :: (store (s64))
 
 
 ...
 
 # test for simple example, should generate sdwa
 # GFX9-LABEL: name:            test5_add_co_sdwa
 # GFX9: V_ADD_CO_U32_sdwa
-# GFX9: V_ADDC_U32_e32
+# GFX9: V_ADDC_U32_e64
 ---
 name:            test5_add_co_sdwa
 tracksRegLiveness: true
 registers:
   - { id: 0, class: vgpr_32, preferred-register: '' }
 liveins:
   - { reg: '$vgpr0', virtual-reg: '%0' }
   - { reg: '$sgpr0_sgpr1', virtual-reg: '%1' }
bb.0:
     %30:vreg_64 = COPY $sgpr0_sgpr1
     %23:vgpr_32 = V_AND_B32_e32 %22, %0, implicit $exec
     %63:vgpr_32, %65:sreg_64_xexec = V_ADD_CO_U32_e64 %30.sub0, %23, 0, implicit $exec
     $vcc = COPY %30
     %31:vreg_64 = COPY killed $vcc
     %64:vgpr_32, %66:sreg_64_xexec = V_ADDC_U32_e64 %30.sub1, %0, %65, 0, implicit $exec
     %62:vreg_64 = REG_SEQUENCE %63, %subreg.sub0, %64, %subreg.sub1
     GLOBAL_STORE_DWORDX2_SADDR %31.sub0, %62, %1, 0, 0, implicit $exec, implicit $exec :: (store (s64))
-

llvm/test/CodeGen/AMDGPU/v_add_u64_pseudo_sdwa.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc -march=amdgcn -mcpu=gfx900 -verify-machineinstrs < %s | FileCheck -check-prefixes=GFX9 %s
 define amdgpu_kernel void @sdwa_test() local_unnamed_addr #0 {
 ; GFX9-LABEL: sdwa_test:
 ; GFX9:       ; %bb.0: ; %bb
 ; GFX9-NEXT:    v_add_u32_e32 v1, 10, v0
 ; GFX9-NEXT:    v_add_u32_e32 v0, 20, v0
-; GFX9-NEXT:    v_and_b32_e32 v0, 0xff, v0
-; GFX9-NEXT:    v_add_co_u32_e32 v0, vcc, v1, v0
+; GFX9-NEXT:    v_add_co_u32_sdwa v0, vcc, v1, v0 dst_sel:DWORD dst_unused:UNUSED_PAD src0_sel:DWORD src1_sel:BYTE_0
 ; GFX9-NEXT:    v_addc_co_u32_e64 v1, s[0:1], 0, 0, vcc
 ; GFX9-NEXT:    global_store_dwordx2 v[0:1], v[0:1], off
 ; GFX9-NEXT:    s_endpgm
 bb:
   %tid = tail call i32 @llvm.amdgcn.workitem.id.x()
   %v0 = add i32 %tid, 10
   %v1 = add i32 %tid, 20
   %v2 = zext i32 %v0 to i64
   %v3 = zext i32 %v1 to i64
   %v.t = and i64 %v3, 255
   %v4 = add i64 %v2, %v.t
   store i64 %v4, i64 addrspace(1) * undef
   ret void
 }
 
 
 define amdgpu_kernel void @test_add_co_sdwa(i64 addrspace(1)* %arg, i32 addrspace(1)* %arg1) #0 {
 ; GFX9-LABEL: test_add_co_sdwa:
 ; GFX9:       ; %bb.0: ; %bb
 ; GFX9-NEXT:    s_load_dwordx4 s[0:3], s[0:1], 0x24
-; GFX9-NEXT:    v_lshlrev_b32_e32 v1, 2, v0
+; GFX9-NEXT:    v_lshlrev_b32_e32 v2, 2, v0
 ; GFX9-NEXT:    v_lshlrev_b32_e32 v3, 3, v0
 ; GFX9-NEXT:    s_waitcnt lgkmcnt(0)
-; GFX9-NEXT:    global_load_dword v2, v1, s[2:3]
-; GFX9-NEXT:    s_nop 0
+; GFX9-NEXT:    global_load_dword v4, v2, s[2:3]
 ; GFX9-NEXT:    global_load_dwordx2 v[0:1], v3, s[0:1]
-; GFX9-NEXT:    s_waitcnt vmcnt(1)
-; GFX9-NEXT:    v_and_b32_e32 v2, 0xff, v2
 ; GFX9-NEXT:    s_waitcnt vmcnt(0)
-; GFX9-NEXT:    v_add_co_u32_e32 v0, vcc, v0, v2
+; GFX9-NEXT:    v_add_co_u32_sdwa v0, vcc, v0, v4 dst_sel:DWORD dst_unused:UNUSED_PAD src0_sel:DWORD src1_sel:BYTE_0
 ; GFX9-NEXT:    v_addc_co_u32_e32 v1, vcc, 0, v1, vcc
 ; GFX9-NEXT:    global_store_dwordx2 v3, v[0:1], s[0:1]
 ; GFX9-NEXT:    s_endpgm
 bb:
   %tmp = tail call i32 @llvm.amdgcn.workitem.id.x()
   %tmp3 = getelementptr inbounds i32, i32 addrspace(1)* %arg1, i32 %tmp
   %tmp4 = load i32, i32 addrspace(1)* %tmp3, align 4
   %tmp5 = and i32 %tmp4, 255

llvm/test/CodeGen/AMDGPU/v_sub_u64_pseudo_sdwa.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc -march=amdgcn -mcpu=gfx900 -verify-machineinstrs < %s | FileCheck -check-prefixes=GFX9 %s
 define amdgpu_kernel void @sdwa_test_sub() local_unnamed_addr #0 {
 ; GFX9-LABEL: sdwa_test_sub:
 ; GFX9:       ; %bb.0: ; %bb
 ; GFX9-NEXT:    v_add_u32_e32 v1, 10, v0
 ; GFX9-NEXT:    v_add_u32_e32 v0, 20, v0
-; GFX9-NEXT:    v_and_b32_e32 v0, 0xff, v0
-; GFX9-NEXT:    v_sub_co_u32_e32 v0, vcc, v1, v0
+; GFX9-NEXT:    v_sub_co_u32_sdwa v0, vcc, v1, v0 dst_sel:DWORD dst_unused:UNUSED_PAD src0_sel:DWORD src1_sel:BYTE_0
 ; GFX9-NEXT:    v_subb_co_u32_e64 v1, s[0:1], 0, 0, vcc
 ; GFX9-NEXT:    global_store_dwordx2 v[0:1], v[0:1], off
 ; GFX9-NEXT:    s_endpgm
 bb:
   %tid = tail call i32 @llvm.amdgcn.workitem.id.x()
   %v0 = add i32 %tid, 10
   %v1 = add i32 %tid, 20
   %v2 = zext i32 %v0 to i64
   %v3 = zext i32 %v1 to i64
   %v.t = and i64 %v3, 255
   %v4 = sub i64 %v2, %v.t
   store i64 %v4, i64 addrspace(1) * undef
   ret void
 }
 
 
 define amdgpu_kernel void @test_sub_co_sdwa(i64 addrspace(1)* %arg, i32 addrspace(1)* %arg1) #0 {
 ; GFX9-LABEL: test_sub_co_sdwa:
 ; GFX9:       ; %bb.0: ; %bb
 ; GFX9-NEXT:    s_load_dwordx4 s[0:3], s[0:1], 0x24
-; GFX9-NEXT:    v_lshlrev_b32_e32 v1, 2, v0
+; GFX9-NEXT:    v_lshlrev_b32_e32 v2, 2, v0
 ; GFX9-NEXT:    v_lshlrev_b32_e32 v3, 3, v0
 ; GFX9-NEXT:    s_waitcnt lgkmcnt(0)
-; GFX9-NEXT:    global_load_dword v2, v1, s[2:3]
-; GFX9-NEXT:    s_nop 0
+; GFX9-NEXT:    global_load_dword v4, v2, s[2:3]
 ; GFX9-NEXT:    global_load_dwordx2 v[0:1], v3, s[0:1]
-; GFX9-NEXT:    s_waitcnt vmcnt(1)
-; GFX9-NEXT:    v_and_b32_e32 v2, 0xff, v2
 ; GFX9-NEXT:    s_waitcnt vmcnt(0)
-; GFX9-NEXT:    v_sub_co_u32_e32 v0, vcc, v0, v2
+; GFX9-NEXT:    v_sub_co_u32_sdwa v0, vcc, v0, v4 dst_sel:DWORD dst_unused:UNUSED_PAD src0_sel:DWORD src1_sel:BYTE_0
 ; GFX9-NEXT:    v_subbrev_co_u32_e32 v1, vcc, 0, v1, vcc
 ; GFX9-NEXT:    global_store_dwordx2 v3, v[0:1], s[0:1]
 ; GFX9-NEXT:    s_endpgm
 bb:
   %tmp = tail call i32 @llvm.amdgcn.workitem.id.x()
   %tmp3 = getelementptr inbounds i32, i32 addrspace(1)* %arg1, i32 %tmp
   %tmp4 = load i32, i32 addrspace(1)* %tmp3, align 4
   %tmp5 = and i32 %tmp4, 255