[AMDGPU] Remove unnecessary v_mov from a register to itself in WQM lowering.

[mjbedy]

Summary

• SI Whole Quad Mode phase is replacing WQM pseudo instructions with v_mov instructions.

While this is necessary for the special handling of moving results out of WWM live ranges,
it is not necessary for WQM live ranges. The result is a v_mov from a register to itself after every
WQM operation. This change uses a COPY psuedo in these cases, which allows the register
allocator to coalesce the moves away.

[mjbedy]

Fix typo in commit message.

[arsenm]

llvm/lib/Target/AMDGPU/SIWholeQuadMode.cpp

876–877

These should only ever have been moves to begin with? Just remove the one exec operand?

[nhaehnle]

This basically LGTM

llvm/lib/Target/AMDGPU/SIWholeQuadMode.cpp

876–877

They were move-like: WQM, SOFT_WQM, V_SET_INACTIVE_xx. I think it makes sense though to just remove that one operand and maybe assert there are no other implicit uses.

[mjbedy]

llvm/lib/Target/AMDGPU/SIWholeQuadMode.cpp

876–877

Now that I look closer at this, I'm not even sure what this loop is doing (or the one in the case above that I modeled it on)? The exec operands are implicit so this doesn't touch them. This is what comes into the phase:

%12:vgpr_32 = WQM %10:vgpr_32, implicit $exec, implicit $exec

And this is what comes out:

%12:vgpr_32 = COPY %10:vgpr_32, implicit $exec, implicit $exec

[foad]

llvm/lib/Target/AMDGPU/SIWholeQuadMode.cpp

876–877

The loop removes any extra explicit operands after the first two, which will become the destination and source of the copy. Currently the SET_INACTIVE instructions have an extra explicit operand called "inactive".

Several passes add an implicit use of exec without checking whether there was one already, so I'm not surprised this pass does not bother to remove any implicit operands. You can see instructions with multiple "implicit $exec" operands in debug output and even in some of our autogenerated lit tests.

[mjbedy]

• Change operand removel code to be less generic, only V_SET_INACTIVE with an undef source needs an operand removed.

[mjbedy]

Remove unintentially included file.

[mjbedy]

llvm/lib/Target/AMDGPU/SIWholeQuadMode.cpp

876–877

I fixed the code to only expect this one case, and added a test for it.

[mjbedy]

Ping.

[nhaehnle]

LGTM, with one minor nitpick.

llvm/test/CodeGen/AMDGPU/wqm.mir

53

*its

llvm/lib/Target/AMDGPU/SIWholeQuadMode.cpp

private:
   const SIRegisterInfo *TRI;
   const GCNSubtarget *ST;
   MachineRegisterInfo *MRI;
   LiveIntervals *LIS;
 
   DenseMap<const MachineInstr *, InstrInfo> Instructions;
   DenseMap<MachineBasicBlock *, BlockInfo> Blocks;
   SmallVector<MachineInstr *, 1> LiveMaskQueries;
+  SmallVector<MachineInstr *, 4> LowerToMovInstrs;
   SmallVector<MachineInstr *, 4> LowerToCopyInstrs;
 
   void printInfo();
 
   void markInstruction(MachineInstr &MI, char Flag,
                        std::vector<WorkItem> &Worklist);
   void markInstructionUses(const MachineInstr &MI, char Flag,
                            std::vector<WorkItem> &Worklist);
for (auto II = MBB.begin(), IE = MBB.end(); II != IE; ++II) {
         SoftWQMInstrs.push_back(&MI);
         continue;
       } else if (Opcode == AMDGPU::WWM) {
         // The WWM intrinsic doesn't make the same guarantee, and plus it needs
         // to be executed in WQM or Exact so that its copy doesn't clobber
         // inactive lanes.
         markInstructionUses(MI, StateWWM, Worklist);
         GlobalFlags |= StateWWM;
-        LowerToCopyInstrs.push_back(&MI);
+        LowerToMovInstrs.push_back(&MI);
         continue;
       } else if (Opcode == AMDGPU::V_SET_INACTIVE_B32 ||
                  Opcode == AMDGPU::V_SET_INACTIVE_B64) {
         III.Disabled = StateWWM;
         MachineOperand &Inactive = MI.getOperand(2);
         if (Inactive.isReg()) {
           if (Inactive.isUndef()) {
             LowerToCopyInstrs.push_back(&MI);
MachineInstr *Copy =
             .addReg(LiveMaskReg);
 
     LIS->ReplaceMachineInstrInMaps(*MI, *Copy);
     MI->eraseFromParent();
   }
 }
 
 void SIWholeQuadMode::lowerCopyInstrs() {
-  for (MachineInstr *MI : LowerToCopyInstrs) {
-    for (unsigned i = MI->getNumExplicitOperands() - 1; i > 1; i--)
-      MI->RemoveOperand(i);
+  for (MachineInstr *MI : LowerToMovInstrs) {
+    assert(MI->getNumExplicitOperands() == 2);
 
     const Register Reg = MI->getOperand(0).getReg();
 
     if (TRI->isVGPR(*MRI, Reg)) {
       const TargetRegisterClass *regClass = Register::isVirtualRegister(Reg)
                                                 ? MRI->getRegClass(Reg)
                                                 : TRI->getPhysRegClass(Reg);
 
       const unsigned MovOp = TII->getMovOpcode(regClass);
       MI->setDesc(TII->get(MovOp));
 
       // And make it implicitly depend on exec (like all VALU movs should do).
       MI->addOperand(MachineOperand::CreateReg(AMDGPU::EXEC, false, true));
     } else {
       MI->setDesc(TII->get(AMDGPU::COPY));
     }
   }
+  for (MachineInstr *MI : LowerToCopyInstrs) {
+    if (MI->getOpcode() == AMDGPU::V_SET_INACTIVE_B32 ||
+        MI->getOpcode() == AMDGPU::V_SET_INACTIVE_B64) {
+      assert(MI->getNumExplicitOperands() == 3);
+      // the only reason we should be here is V_SET_INACTIVE has
+      // an undef input so it is being replaced by a simple copy.
+      // There should be a second undef source that we should remove.
+      assert(MI->getOperand(2).isUndef());
+      MI->RemoveOperand(2);
+      MI->untieRegOperand(1);
+    } else {
+      assert(MI->getNumExplicitOperands() == 2);
+    }
+
+    MI->setDesc(TII->get(AMDGPU::COPY));
+  }
 }
 
 bool SIWholeQuadMode::runOnMachineFunction(MachineFunction &MF) {
   Instructions.clear();
   Blocks.clear();
   LiveMaskQueries.clear();
   LowerToCopyInstrs.clear();
+  LowerToMovInstrs.clear();
   CallingConv = MF.getFunction().getCallingConv();
 
   ST = &MF.getSubtarget<GCNSubtarget>();
 
   TII = ST->getInstrInfo();
   TRI = &TII->getRegisterInfo();
   MRI = &MF.getRegInfo();
   LIS = &getAnalysis<LiveIntervals>();
 
   char GlobalFlags = analyzeFunction(MF);
   unsigned LiveMaskReg = 0;
   unsigned Exec = ST->isWave32() ? AMDGPU::EXEC_LO : AMDGPU::EXEC;
   if (!(GlobalFlags & StateWQM)) {
     lowerLiveMaskQueries(Exec);
-    if (!(GlobalFlags & StateWWM) && LowerToCopyInstrs.empty())
+    if (!(GlobalFlags & StateWWM) && LowerToCopyInstrs.empty() && LowerToMovInstrs.empty())
       return !LiveMaskQueries.empty();
   } else {
     // Store a copy of the original live mask when required
     MachineBasicBlock &Entry = MF.front();
     MachineBasicBlock::iterator EntryMI = Entry.getFirstNonPHI();
 
     if (GlobalFlags & StateExact || !LiveMaskQueries.empty()) {
       LiveMaskReg = MRI->createVirtualRegister(TRI->getBoolRC());

llvm/test/CodeGen/AMDGPU/wqm.ll

 
 ; Check that WQM is triggered by the wqm intrinsic.
 ;
 ;CHECK-LABEL: {{^}}test5:
 ;CHECK: s_wqm_b64 exec, exec
 ;CHECK: buffer_load_dword
 ;CHECK: buffer_load_dword
 ;CHECK: v_add_f32_e32
+; WQM was inserting an unecessary v_mov to self after the v_add. Make sure this
+; does not happen - the v_add should write the return reg directly.
+;CHECK-NOT: v_mov_b32_e32
 define amdgpu_ps float @test5(i32 inreg %idx0, i32 inreg %idx1) {
 main_body:
   %src0 = call float @llvm.amdgcn.buffer.load.f32(<4 x i32> undef, i32 %idx0, i32 0, i1 0, i1 0)
   %src1 = call float @llvm.amdgcn.buffer.load.f32(<4 x i32> undef, i32 %idx1, i32 0, i1 0, i1 0)
   %out = fadd float %src0, %src1
   %out.0 = call float @llvm.amdgcn.wqm.f32(float %out)
   ret float %out.0
 }

llvm/test/CodeGen/AMDGPU/wqm.mir

bb.0:
     S_CMP_LT_I32 0, %0, implicit-def $scc
     %12 = V_ADD_I32_e32 %3, %3, implicit-def $vcc, implicit $exec
     %5 = S_CSELECT_B32 %2, %1, implicit $scc
     %11 = V_ADD_I32_e32 %5, %12, implicit-def $vcc, implicit $exec
     $vgpr0 = WWM %11, implicit $exec
     SI_RETURN_TO_EPILOG $vgpr0
 
 ...
+
+---
+# V_SET_INACTIVE, when it's second operand is undef, is replaced by a
+# COPY by si-wqm. Ensure the instruction is removed.
+#CHECK-NOT: V_SET_INACTIVE
+name:            no_cfg
+alignment:       1
+exposesReturnsTwice: false
+legalized:       false
+regBankSelected: false
+selected:        false
+failedISel:      false
+tracksRegLiveness: true
+hasWinCFI:       false
+registers:
+  - { id: 0, class: sgpr_32, preferred-register: '' }
+  - { id: 1, class: sgpr_32, preferred-register: '' }
+  - { id: 2, class: sgpr_32, preferred-register: '' }
+  - { id: 3, class: sgpr_32, preferred-register: '' }
+  - { id: 4, class: sgpr_32, preferred-register: '' }
+  - { id: 5, class: sgpr_128, preferred-register: '' }
+  - { id: 6, class: sgpr_128, preferred-register: '' }
+  - { id: 7, class: sreg_32, preferred-register: '' }
+  - { id: 8, class: vreg_64, preferred-register: '' }
+  - { id: 9, class: sreg_32, preferred-register: '' }
+  - { id: 10, class: vgpr_32, preferred-register: '' }
+  - { id: 11, class: vgpr_32, preferred-register: '' }
+  - { id: 12, class: sreg_32, preferred-register: '' }
+  - { id: 13, class: vgpr_32, preferred-register: '' }
+  - { id: 14, class: vgpr_32, preferred-register: '' }
+  - { id: 15, class: vgpr_32, preferred-register: '' }
+  - { id: 16, class: vgpr_32, preferred-register: '' }
+liveins:
+  - { reg: '$sgpr0', virtual-reg: '%0' }
+  - { reg: '$sgpr1', virtual-reg: '%1' }
+  - { reg: '$sgpr2', virtual-reg: '%2' }
+  - { reg: '$sgpr3', virtual-reg: '%3' }
+body:             |
+  bb.0:
+    liveins: $sgpr0, $sgpr1, $sgpr2, $sgpr3
+
+    %3:sgpr_32 = COPY $sgpr3
+    %2:sgpr_32 = COPY $sgpr2
+    %1:sgpr_32 = COPY $sgpr1
+    %0:sgpr_32 = COPY $sgpr0
+    %6:sgpr_128 = REG_SEQUENCE %0, %subreg.sub0, %1, %subreg.sub1, %2, %subreg.sub2, %3, %subreg.sub3
+    %5:sgpr_128 = COPY %6
+    %7:sreg_32 = S_MOV_B32 0
+    %8:vreg_64 = BUFFER_LOAD_DWORDX2_OFFSET %6, %7, 0, 0, 0, 0, 0, 0, implicit $exec
+    %16:vgpr_32 = COPY %8.sub1
+    %11:vgpr_32 = COPY %16
+    %10:vgpr_32 = V_SET_INACTIVE_B32 %11, undef %12:sreg_32, implicit $exec
+    %14:vgpr_32 = COPY %7
+    %13:vgpr_32 = V_MOV_B32_dpp %14, killed %10, 323, 12, 15, 0, implicit $exec
+    early-clobber %15:vgpr_32 = WWM killed %13, implicit $exec
+    BUFFER_STORE_DWORD_OFFSET_exact killed %15, %6, %7, 4, 0, 0, 0, 0, 0, implicit $exec
+    S_ENDPGM 0
+
+...