[AMDGPU] Reject moving PHI to VALU if the only VGPR input originated from move immediate

[alex-t]

Summary

PHIs result register class is set to VGPR or SGPR depending on the cross block value divergence.

       In some cases uniform PHI need to be converted to return VGPR to prevent the oddnumber of moves values from VGPR to SGPR and back.
       PHI should certainly return VGPR if it has at least one VGPR input. This change adds the exception. 
       We don't want to convert uniform PHI to VGPRs in case the only VGPR input is a VGPR to SGPR COPY and definition od the 
       source VGPR in this COPY is move immediate.
                       
bb.0:
  
   %0:vgpr_32 = V_MOV_B32_e32 0, implicit $exec
   %2:sreg_32 = .....

bb.1:
   %3:sreg_32 = PHI %1, %bb.3, %2, %bb.1
   S_BRANCH %bb.3

bb.3:
   %1:sreg_32 = COPY %0 
   S_BRANCH %bb.2

[alex-t]

test added

[rampitec]

Every mov isMoveImmediate(), it is just a property on MCDesc.
I also assume it relies on the folding later. If that folding will not happen for whatever reason it will be a broken IR. Right?

[alex-t]

Explicit check move source operand to be immediate.

[alex-t]

In D80434#2051336, @rampitec wrote:

Every mov isMoveImmediate(), it is just a property on MCDesc.
I also assume it relies on the folding later. If that folding will not happen for whatever reason it will be a broken IR. Right?

I meant the move a constant value to VGPR. In this particular case it was moving 0. Then GVN force the VGPR to be reused multiple times along the basic block and it's successors.
The PHI in question just needed value 0. No other reasons to use VGPR.

[rampitec]

You do not even need a loop to get PHI. A PHI might occur after an simple IF.
Nevertheless, patch retains an illegal copy which it is supposed to remove. The fact that it works in this testcase does not mean it will always work.
I would suggest to fold the immediate copy instead. I.e. you can replace COPY with an S_MOV_B32 (given it fits 32 bit of course).

[alex-t]

In D80434#2051602, @rampitec wrote:

You do not even need a loop to get PHI. A PHI might occur after an simple IF.
Nevertheless, patch retains an illegal copy which it is supposed to remove. The fact that it works in this testcase does not mean it will always work.
I would suggest to fold the immediate copy instead. I.e. you can replace COPY with an S_MOV_B32 (given it fits 32 bit of course).

The following code does it in one of the next iterations

// If we are just copying an immediate, we can replace the copy with
// s_mov_b32.
if (isSafeToFoldImmIntoCopy(&MI, DefMI, TII, SMovOp, Imm)) {
  MI.getOperand(1).ChangeToImmediate(Imm);
  MI.addImplicitDefUseOperands(MF);
  MI.setDesc(TII->get(SMovOp));
  break;
}

[rampitec]

In D80434#2051603, @alex-t wrote:

In D80434#2051602, @rampitec wrote:

You do not even need a loop to get PHI. A PHI might occur after an simple IF.
Nevertheless, patch retains an illegal copy which it is supposed to remove. The fact that it works in this testcase does not mean it will always work.
I would suggest to fold the immediate copy instead. I.e. you can replace COPY with an S_MOV_B32 (given it fits 32 bit of course).

The following code does it in one of the next iterations

// If we are just copying an immediate, we can replace the copy with
// s_mov_b32.
if (isSafeToFoldImmIntoCopy(&MI, DefMI, TII, SMovOp, Imm)) {
  MI.getOperand(1).ChangeToImmediate(Imm);
  MI.addImplicitDefUseOperands(MF);
  MI.setDesc(TII->get(SMovOp));
  break;
}

What if not isSafeToFoldImmIntoCopy?

[alex-t]

In D80434#2051618, @rampitec wrote:

In D80434#2051603, @alex-t wrote:

In D80434#2051602, @rampitec wrote:

You do not even need a loop to get PHI. A PHI might occur after an simple IF.
Nevertheless, patch retains an illegal copy which it is supposed to remove. The fact that it works in this testcase does not mean it will always work.
I would suggest to fold the immediate copy instead. I.e. you can replace COPY with an S_MOV_B32 (given it fits 32 bit of course).

The following code does it in one of the next iterations

// If we are just copying an immediate, we can replace the copy with
// s_mov_b32.
if (isSafeToFoldImmIntoCopy(&MI, DefMI, TII, SMovOp, Imm)) {
  MI.getOperand(1).ChangeToImmediate(Imm);
  MI.addImplicitDefUseOperands(MF);
  MI.setDesc(TII->get(SMovOp));
  break;
}

What if not isSafeToFoldImmIntoCopy?

Okay. isSafeToFoldImmIntoCopy returns 'false' by one of the following:

if (Copy->getOpcode() != AMDGPU::COPY)
   return false;

 if (!MoveImm->isMoveImmediate())
   return false;

 const MachineOperand *ImmOp =
     TII->getNamedOperand(*MoveImm, AMDGPU::OpName::src0);
 if (!ImmOp->isImm())
   return false;

 // FIXME: Handle copies with sub-regs.
 if (Copy->getOperand(0).getSubReg())
   return false;

 switch (MoveImm->getOpcode()) {
 default:
   return false;

The latter 2 conditions - subregs and exact move opcode - are not handled in my code.
So, what would you suggest:

1. guard with if (isSafeToFoldImmIntoCopy(...))
2. add subreg and exact opcode check to my 'if' ?

[alex-t]

Explicit call to isSafeToFoldImmIntoCopy added. Corresponding COPY converted to S_MOV right away.
2 test cases added to check for copy to subreg and inappropriate move opcodes.

[rampitec]

llvm/test/CodeGen/AMDGPU/phi-vgpr-input-moveimm.mir

36

Please fix the label to be a function name.

69

Here too.

72

Does it compile with a white space in function's name?

[alex-t]

test corrected

llvm/test/CodeGen/AMDGPU/phi-vgpr-input-moveimm.mir

72

Yes. Though the whitespace it a typo it compiles and same name is in output file.

[rampitec]

LGTM

llvm/lib/Target/AMDGPU/SIFixSGPRCopies.cpp

if (TRI->isVectorRegister(*MRI, InputReg)) {
         if (TRI->isSGPRClass(RC))
           continue;
       }
       hasVGPRInput = true;
       break;
     }
     else if (Def->isCopy() &&
       TRI->isVectorRegister(*MRI, Def->getOperand(1).getReg())) {
+      Register SrcReg = Def->getOperand(1).getReg();
+      MachineInstr *SrcDef = MRI->getVRegDef(SrcReg);
+      unsigned SMovOp;
+      int64_t Imm;
+      if (!isSafeToFoldImmIntoCopy(Def, SrcDef, TII, SMovOp, Imm)) {
         hasVGPRInput = true;
         break;
+      } else {
+        // Formally, if we did not do this right away
+        // it would be done on the next iteration of the
+        // runOnMachineFunction main loop. But why not if we can?
+        MachineFunction *MF = MI.getParent()->getParent();
+        Def->getOperand(1).ChangeToImmediate(Imm);
+        Def->addImplicitDefUseOperands(*MF);
+        Def->setDesc(TII->get(SMovOp));
+      }
     }
   }
 
   if ((!TRI->isVectorRegister(*MRI, PHIRes) &&
        RC0 != &AMDGPU::VReg_1RegClass) &&
     (hasVGPRInput || numVGPRUses > 1)) {
     LLVM_DEBUG(dbgs() << "Fixing PHI: " << MI);
     TII->moveToVALU(MI);

llvm/test/CodeGen/AMDGPU/phi-vgpr-input-moveimm.mir

+# RUN: llc  -march=amdgcn -mcpu=gfx900 -verify-machineinstrs -run-pass=si-fix-sgpr-copies -o - %s | FileCheck -check-prefix=GCN %s
+---
+# GCN_LABEL: bb.2:
+# GCN-NOT: %{{[0-9]+}}:vgpr_32 = PHI %{{[0-9]+}}, %bb.3, %{{[0-9]+}}, %bb.1
+# GCN:     %{{[0-9]+}}:sreg_32 = PHI %{{[0-9]+}}, %bb.3, %{{[0-9]+}}, %bb.1
+
+name:            phi_moveimm_input
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    successors: %bb.1
+    liveins: $sgpr0, $sgpr1
+
+    %0:vgpr_32 = V_MOV_B32_e32 0, implicit $exec
+
+    %4:sreg_32 = COPY $sgpr0
+    %5:sreg_32 = COPY $sgpr1
+
+  bb.1:
+    successors: %bb.2
+    %2:sreg_32 =  S_ADD_U32 %4, %5, implicit-def $scc
+    S_BRANCH %bb.2
+
+  bb.2:
+    successors: %bb.3
+    %3:sreg_32 = PHI %1, %bb.3, %2, %bb.1
+    S_BRANCH %bb.3
+
+  bb.3:
+    successors: %bb.2
+    %1:sreg_32 = COPY %0
+    S_BRANCH %bb.2
+...
+
+---
+# GCN_LABEL: bb.2:
+# GCN-NOT: %{{[0-9]+}}:sreg_64 = PHI %{{[0-9]+}}, %bb.3, %{{[0-9]+}}, %bb.1
+# GCN: %{{[0-9]+}}:vreg_64 = PHI %{{[0-9]+}}, %bb.3, %{{[0-9]+}}, %bb.1
+name:            phi_moveimm_subreg_input
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    successors: %bb.1
+    liveins: $sgpr0, $sgpr1
+
+    %0:vgpr_32 = V_MOV_B32_e32 0, implicit $exec
+
+    %4:sreg_32 = COPY $sgpr0
+    %5:sreg_32 = COPY $sgpr1
+
+  bb.1:
+    successors: %bb.2
+    undef %2.sub0:sreg_64 =  S_ADD_U32 %4, %5, implicit-def $scc
+    S_BRANCH %bb.2
+
+  bb.2:
+    successors: %bb.3
+    %3:sreg_64 = PHI %1, %bb.3, %2, %bb.1
+    S_BRANCH %bb.3
+
+  bb.3:
+    successors: %bb.2
+    undef %1.sub0:sreg_64 = COPY %0
+    S_BRANCH %bb.2
+...
+
+
+---
+# GCN_LABEL: bb.2:
+# GCN-NOT: %{{[0-9]+}}:sreg_32 = PHI %{{[0-9]+}}, %bb.3, %{{[0-9]+}}, %bb.1
+# GCN: %{{[0-9]+}}:vgpr_32 = PHI %{{[0-9]+}}, %bb.3, %{{[0-9]+}}, %bb.1
+name:            phi_moveimm_bad opcode_input
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    successors: %bb.1
+    liveins: $sgpr0, $sgpr1, $vgpr0
+    %6:vgpr_32 = COPY $vgpr0
+    %0:vgpr_32 = V_MOV_B32_sdwa 0, %6:vgpr_32, 0, 5, 2, 4,  implicit $exec, implicit %6:vgpr_32(tied-def 0)
+
+    %4:sreg_32 = COPY $sgpr0
+    %5:sreg_32 = COPY $sgpr1
+
+  bb.1:
+
+    successors: %bb.2
+    %2:sreg_32 =  S_ADD_U32 %4, %5, implicit-def $scc
+    S_BRANCH %bb.2
+  bb.2:
+    successors: %bb.3
+    %3:sreg_32 = PHI %1, %bb.3, %2, %bb.1
+    S_BRANCH %bb.3
+  bb.3:
+    successors: %bb.2
+    %1:sreg_32 = COPY %0
+    S_BRANCH %bb.2
+...