[AMDGPU] Lower VGPR to physical SGPR COPY to S_MOV_B32 if VGPR contains the compile time constant

[alex-t]

Summary

Sometimes we have a constant value loaded to VGPR. In case we further
need to rematrerialize it in the physical scalar register we may avoid VGPR to
SGPR copy replacing it with S_MOV_B32.

[JonChesterfield]

Test case from D139852 (with code sequence slightly rearranged) passes with this, e.g.

; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
; RUN: llc -O0 -mcpu=gfx1030 < %s | FileCheck %s

target triple = "amdgcn-amd-amdhsa"

; Unknown functions are conservatively passed all implicit parameters
declare void @unknown_call()
; Use the same constant as a sgpr parameter (for the kernel id) and for a vector operation
define protected amdgpu_kernel void @kern(ptr %addr) !llvm.amdgcn.lds.kernel.id !0 {
; CHECK-LABEL: kern:
; CHECK:       ; %bb.0:
; CHECK-NEXT:    s_mov_b32 s32, 0
; CHECK-NEXT:    s_add_u32 s12, s12, s17
; CHECK-NEXT:    s_addc_u32 s13, s13, 0
; CHECK-NEXT:    s_setreg_b32 hwreg(HW_REG_FLAT_SCR_LO), s12
; CHECK-NEXT:    s_setreg_b32 hwreg(HW_REG_FLAT_SCR_HI), s13
; CHECK-NEXT:    s_add_u32 s0, s0, s17
; CHECK-NEXT:    s_addc_u32 s1, s1, 0
; CHECK-NEXT:    v_writelane_b32 v40, s16, 0
; CHECK-NEXT:    s_mov_b32 s13, s15
; CHECK-NEXT:    s_mov_b32 s12, s14
; CHECK-NEXT:    v_readlane_b32 s14, v40, 0
; CHECK-NEXT:    s_mov_b64 s[16:17], s[8:9]
; CHECK-NEXT:    s_load_dwordx2 s[8:9], s[16:17], 0x0
; CHECK-NEXT:    v_mov_b32_e32 v5, 42
; CHECK-NEXT:    s_waitcnt lgkmcnt(0)
; CHECK-NEXT:    v_mov_b32_e32 v3, s8
; CHECK-NEXT:    v_mov_b32_e32 v4, s9
; CHECK-NEXT:    flat_store_dword v[3:4], v5
; CHECK-NEXT:    s_mov_b64 s[18:19], 8
; CHECK-NEXT:    s_mov_b32 s8, s16
; CHECK-NEXT:    s_mov_b32 s9, s17
; CHECK-NEXT:    s_mov_b32 s16, s18
; CHECK-NEXT:    s_mov_b32 s15, s19
; CHECK-NEXT:    s_add_u32 s8, s8, s16
; CHECK-NEXT:    s_addc_u32 s15, s9, s15
; CHECK-NEXT:    ; kill: def $sgpr8 killed $sgpr8 def $sgpr8_sgpr9
; CHECK-NEXT:    s_mov_b32 s9, s15
; CHECK-NEXT:    s_getpc_b64 s[16:17]
; CHECK-NEXT:    s_add_u32 s16, s16, unknown_call@gotpcrel32@lo+4
; CHECK-NEXT:    s_addc_u32 s17, s17, unknown_call@gotpcrel32@hi+12
; CHECK-NEXT:    s_load_dwordx2 s[16:17], s[16:17], 0x0
; CHECK-NEXT:    s_mov_b64 s[22:23], s[2:3]
; CHECK-NEXT:    s_mov_b64 s[20:21], s[0:1]
; CHECK-NEXT:    s_mov_b32 s15, 20
; CHECK-NEXT:    v_lshlrev_b32_e64 v2, s15, v2
; CHECK-NEXT:    s_mov_b32 s15, 10
; CHECK-NEXT:    v_lshlrev_b32_e64 v1, s15, v1
; CHECK-NEXT:    v_or3_b32 v31, v0, v1, v2
; CHECK-NEXT:    s_mov_b32 s15, 42
; CHECK-NEXT:    s_mov_b64 s[0:1], s[20:21]
; CHECK-NEXT:    s_mov_b64 s[2:3], s[22:23]
; CHECK-NEXT:    s_waitcnt lgkmcnt(0)
; CHECK-NEXT:    s_swappc_b64 s[30:31], s[16:17]
; CHECK-NEXT:    s_endpgm
  store i32 42, ptr %addr
  call fastcc void @unknown_call()
  ret void
}

!0 = !{i32 42}

if you add that to the commit I'll be pleased to accept. isMoveImmediate() is a helpful function to have available.

[alex-t]

test added

[alex-t]

test renamed

[JonChesterfield]

Brilliant, thank you!

[arsenm]

llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp

852

Don't use value MachineOperands

853

This isn't guaranteed by isMoveImmediate

[arsenm]

llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp

854–856

Could also have 64-bit VGPR immediate. We have a pseudo for it

[JonChesterfield]

Sad times. @arsenm I'm guessing you're reluctant to ship D139852 in the meantime? Could check in this test with it and revert that block when this lands later.

[alex-t]

changed as requested

[JonChesterfield]

llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp

857

Not sure the signed comparison works here. INT64_MIN is smaller than UINT32_MAX. Checked getImm() and it does return a signed value, so I guess we either convert to unsigned then compare, or truncate to int32_t and check that is equal to the original

[alex-t]

assign getImm() result to unsigned for implicit conversion

[JonChesterfield]

Thanks! @arsenm?

[arsenm]

llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp

862

First of all, you should be just doing addOperand and forwarding whatever was there originally. No need to muck about with conversions or immediates

The 64-bit case is not solved by simply truncating the immediate. It's materializing a 64-bit value. I'd think you'd be hard pressed to write a a testcase without using MIR (which probably should be done)

[JonChesterfield]

llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp

862

If we're copying to a single sgpr and the value doesn't fit, codegen has already gone wrong. We may as well fall through to the current behaviour (which is to miscompile).

If this code path might be copying to pairs of sgprs (I can't tell from the immediate surroundings) then this needs to be significantly more complicated, and probably pick up testing and so forth, but equally that's also broken at present.

I'd rather emit the mov at ISel instead of here.

[alex-t]

add operand instead of adding immediate

[alex-t]

reviewers comments addressed

[alex-t]

llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp

862

Since we cannot rematerialize the 64bit value in 32bit SGPR we already have incorrect MIR if we have 32bit dest register

[alex-t]

llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp

862

Since we cannot rematerialize the 64bit value in 32bit SGPR we already have incorrect MIR if we have 32bit dest register

Please ignore this comment. It was added accidentally. I don't know how to remove it :)

[arsenm]

llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp

854

Not isImm, but !isReg

855–856

Should prefer to get the class and size from the vreg. physregs are expensive

[alex-t]

register class and move size from the virtual source register
operand type check is loosen to !isReg()

llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp

public:
   unsigned getNextVGPRToSGPRCopyId() { return ++NextVGPRToSGPRCopyID; }
   bool needToBeConvertedToVALU(V2SCopyInfo *I);
   void analyzeVGPRToSGPRCopy(MachineInstr *MI);
   void lowerVGPR2SGPRCopies(MachineFunction &MF);
   // Handles copies which source register is:
   // 1. Physical register
   // 2. AGPR
   // 3. Defined by the instruction the merely moves the immediate
-  bool lowerSpecialCase(MachineInstr &MI);
+  bool lowerSpecialCase(MachineInstr &MI, MachineBasicBlock::iterator &I);
 
   void processPHINode(MachineInstr &MI);
 
   StringRef getPassName() const override { return "SI Fix SGPR copies"; }
 
   void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.addRequired<MachineDominatorTree>();
     AU.addPreserved<MachineDominatorTree>();
for (MachineBasicBlock::iterator I = MBB->begin(), E = MBB->end(); I != E;
             continue;
 
           // Collect those not changed to try them after VGPR to SGPR copies
           // lowering as there will be more opportunities.
           S2VCopies.push_back(&MI);
         }
         if (!isVGPRToSGPRCopy(SrcRC, DstRC, *TRI))
           continue;
-        if (lowerSpecialCase(MI))
+        if (lowerSpecialCase(MI, I))
           continue;
 
         analyzeVGPRToSGPRCopy(&MI);
 
         break;
       }
       case AMDGPU::INSERT_SUBREG:
       case AMDGPU::PHI:
void SIFixSGPRCopies::processPHINode(MachineInstr &MI) {
   }
 
   // Propagate register class back to PHI operands which are PHI themselves.
   while (!PHIOperands.empty()) {
     processPHINode(*PHIOperands.pop_back_val());
   }
 }
 
-bool SIFixSGPRCopies::lowerSpecialCase(MachineInstr &MI) {
+bool SIFixSGPRCopies::lowerSpecialCase(MachineInstr &MI,
+                                       MachineBasicBlock::iterator &I) {
   Register DstReg = MI.getOperand(0).getReg();
   Register SrcReg = MI.getOperand(1).getReg();
   if (!DstReg.isVirtual()) {
     // If the destination register is a physical register there isn't
     // really much we can do to fix this.
     // Some special instructions use M0 as an input. Some even only use
     // the first lane. Insert a readfirstlane and hope for the best.
     if (DstReg == AMDGPU::M0 &&
         TRI->hasVectorRegisters(MRI->getRegClass(SrcReg))) {
       Register TmpReg =
           MRI->createVirtualRegister(&AMDGPU::SReg_32_XM0RegClass);
       BuildMI(*MI.getParent(), MI, MI.getDebugLoc(),
               TII->get(AMDGPU::V_READFIRSTLANE_B32), TmpReg)
           .add(MI.getOperand(1));
       MI.getOperand(1).setReg(TmpReg);
+    } else {
+      MachineInstr *DefMI = MRI->getVRegDef(SrcReg);
+      if (DefMI && DefMI->isMoveImmediate()) {
+        MachineOperand SrcConst = DefMI->getOperand(AMDGPU::getNamedOperandIdx(
+            DefMI->getOpcode(), AMDGPU::OpName::src0));
+        if (!SrcConst.isReg()) {
+          const TargetRegisterClass *SrcRC = MRI->getRegClass(SrcReg);
+          unsigned MoveSize = TRI->getRegSizeInBits(*SrcRC);
+          unsigned MoveOp =
+              MoveSize == 64 ? AMDGPU::S_MOV_B64 : AMDGPU::S_MOV_B32;
+          BuildMI(*MI.getParent(), MI, MI.getDebugLoc(), TII->get(MoveOp),
+                  DstReg)
+              .add(SrcConst);
+          I = std::next(I);
+          if (MRI->hasOneUse(SrcReg))
+            DefMI->eraseFromParent();
+          MI.eraseFromParent();
+        }
+      }
     }
     return true;
   }
   if (!SrcReg.isVirtual() || TRI->isAGPR(*MRI, SrcReg)) {
     TII->moveToVALU(MI, MDT);
     return true;
   }
 

llvm/lib/Target/AMDGPU/SIInstructions.td

 // SIFoldOperands pass to enable folding of inline immediates.
 def V_MOV_B64_PSEUDO : VPseudoInstSI <(outs VReg_64:$vdst),
                                       (ins VSrc_b64:$src0)> {
   let isReMaterializable = 1;
   let isAsCheapAsAMove = 1;
   let isMoveImm = 1;
   let SchedRW = [Write64Bit];
   let Size = 16; // Needs maximum 2 v_mov_b32 instructions 8 byte long each.
+  let UseNamedOperandTable = 1;
 }
 
 // 64-bit vector move with dpp. Expanded post-RA.
 def V_MOV_B64_DPP_PSEUDO : VOP_DPP_Pseudo <"v_mov_b64_dpp", VOP_I64_I64> {
   let Size = 16; // Requires two 8-byte v_mov_b32_dpp to complete.
 }
 
 // 64-bit scalar move immediate instruction. This is used to avoid subregs

llvm/test/CodeGen/AMDGPU/vgpr_constant64_to_sgpr.mir

+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -O0 -mtriple=amdgcn-amd-amdhsa --global-isel=0 --run-pass=si-fix-sgpr-copies -verify-machineinstrs -o - %s | FileCheck --check-prefix=GCN %s
+
+---
+name:            test_64imm
+registers:
+  - { id: 1, class: vreg_64_align2 }
+liveins:
+  - { reg: '$sgpr4_sgpr5', virtual-reg: '%1' }
+body:             |
+  bb.0:
+
+    ; GCN-LABEL: name: test_64imm
+    ; GCN: $sgpr8_sgpr9 = S_MOV_B64 4607182418800017408
+    %1 = V_MOV_B64_PSEUDO 4607182418800017408, implicit $exec
+    $sgpr8_sgpr9 = COPY %1
+...
+

llvm/test/CodeGen/AMDGPU/vgpr_constant_to_sgpr.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -O0 -mcpu=gfx1030 < %s | FileCheck %s
+
+target triple = "amdgcn-amd-amdhsa"
+
+; Unknown functions are conservatively passed all implicit parameters
+declare void @unknown_call()
+; Use the same constant as a sgpr parameter (for the kernel id) and for a vector operation
+define protected amdgpu_kernel void @kern(ptr %addr) !llvm.amdgcn.lds.kernel.id !0 {
+; CHECK-LABEL: kern:
+; CHECK:       ; %bb.0:
+; CHECK-NEXT:    s_mov_b32 s32, 0
+; CHECK-NEXT:    s_add_u32 s12, s12, s17
+; CHECK-NEXT:    s_addc_u32 s13, s13, 0
+; CHECK-NEXT:    s_setreg_b32 hwreg(HW_REG_FLAT_SCR_LO), s12
+; CHECK-NEXT:    s_setreg_b32 hwreg(HW_REG_FLAT_SCR_HI), s13
+; CHECK-NEXT:    s_add_u32 s0, s0, s17
+; CHECK-NEXT:    s_addc_u32 s1, s1, 0
+; CHECK-NEXT:    v_writelane_b32 v40, s16, 0
+; CHECK-NEXT:    s_mov_b32 s13, s15
+; CHECK-NEXT:    s_mov_b32 s12, s14
+; CHECK-NEXT:    v_readlane_b32 s14, v40, 0
+; CHECK-NEXT:    s_mov_b64 s[16:17], s[8:9]
+; CHECK-NEXT:    s_load_dwordx2 s[8:9], s[16:17], 0x0
+; CHECK-NEXT:    v_mov_b32_e32 v5, 42
+; CHECK-NEXT:    s_waitcnt lgkmcnt(0)
+; CHECK-NEXT:    v_mov_b32_e32 v3, s8
+; CHECK-NEXT:    v_mov_b32_e32 v4, s9
+; CHECK-NEXT:    flat_store_dword v[3:4], v5
+; CHECK-NEXT:    s_mov_b64 s[18:19], 8
+; CHECK-NEXT:    s_mov_b32 s8, s16
+; CHECK-NEXT:    s_mov_b32 s9, s17
+; CHECK-NEXT:    s_mov_b32 s16, s18
+; CHECK-NEXT:    s_mov_b32 s15, s19
+; CHECK-NEXT:    s_add_u32 s8, s8, s16
+; CHECK-NEXT:    s_addc_u32 s15, s9, s15
+; CHECK-NEXT:    ; kill: def $sgpr8 killed $sgpr8 def $sgpr8_sgpr9
+; CHECK-NEXT:    s_mov_b32 s9, s15
+; CHECK-NEXT:    s_getpc_b64 s[16:17]
+; CHECK-NEXT:    s_add_u32 s16, s16, unknown_call@gotpcrel32@lo+4
+; CHECK-NEXT:    s_addc_u32 s17, s17, unknown_call@gotpcrel32@hi+12
+; CHECK-NEXT:    s_load_dwordx2 s[16:17], s[16:17], 0x0
+; CHECK-NEXT:    s_mov_b64 s[22:23], s[2:3]
+; CHECK-NEXT:    s_mov_b64 s[20:21], s[0:1]
+; CHECK-NEXT:    s_mov_b32 s15, 20
+; CHECK-NEXT:    v_lshlrev_b32_e64 v2, s15, v2
+; CHECK-NEXT:    s_mov_b32 s15, 10
+; CHECK-NEXT:    v_lshlrev_b32_e64 v1, s15, v1
+; CHECK-NEXT:    v_or3_b32 v31, v0, v1, v2
+; CHECK-NEXT:    s_mov_b32 s15, 42
+; CHECK-NEXT:    s_mov_b64 s[0:1], s[20:21]
+; CHECK-NEXT:    s_mov_b64 s[2:3], s[22:23]
+; CHECK-NEXT:    s_waitcnt lgkmcnt(0)
+; CHECK-NEXT:    s_swappc_b64 s[30:31], s[16:17]
+; CHECK-NEXT:    s_endpgm
+  store i32 42, ptr %addr
+  call fastcc void @unknown_call()
+  ret void
+}
+
+!0 = !{i32 42}