AMDGPU/SI: Don't fix a PHI under uniform branch in SIFixSGPRCopies only when sources and destination are all sgprs

[cfang]

Summary

We still have to fix the PHI if there is at least one VGPR operand.

[arsenm]

lib/Target/AMDGPU/SIFixSGPRCopies.cpp

608 ↗

(On Diff #104312)

I think you should move the below phiHasVGPROperands check first rather than adding a second one

test/CodeGen/AMDGPU/vgpr-to-sgpr-phi.ll

3 ↗

(On Diff #104312)

GCN check prefix

7 ↗

(On Diff #104312)

It may be better to reformulate as a kernel for now, which will require a non-argument source for the VGPR

7–9 ↗

(On Diff #104312)

Run instnamer on test. Also can be merged

30 ↗

(On Diff #104312)

Drop extra string attributes

34 ↗

(On Diff #104312)

Drop all metadata

[cfang]

[cfang]

Update based on Matt's review.

For the test:

1. need details on how to change to a kernel, especially what the argument should be.
   • I tried a few options, but it seems we could not reproduce the issue with the kernel version, possibly because code is optimized away.

2. not clear what to merge!

[arsenm]

test/CodeGen/AMDGPU/vgpr-to-sgpr-phi.ll

7 ↗

(On Diff #104503)

Does it still reproduce if you make it a kernel?

7–9 ↗

(On Diff #104312)

It can merge into one of the other test files for this pass

[cfang]

test/CodeGen/AMDGPU/vgpr-to-sgpr-phi.ll

7 ↗

(On Diff #104503)

The issue is not reproducible if I make it a kernel.

7–9 ↗

(On Diff #104312)

Can you be more specific which test should I merge to? It is not clear to me which tests are for this pass (simply guess from the names).
If I place an assert in the place I am working on, I saw more than twenty tests failures.

Which test is for the following piece of code?

// We don't need to fix the PHI if the common dominator of the
// two incoming blocks terminates with a uniform branch.
if (MI.getNumExplicitOperands() == 5) {
  MachineBasicBlock *MBB0 = MI.getOperand(2).getMBB();
  MachineBasicBlock *MBB1 = MI.getOperand(4).getMBB();

  if (!predsHasDivergentTerminator(MBB0, TRI) &&
      !predsHasDivergentTerminator(MBB1, TRI)) {
    DEBUG(dbgs() << "Not fixing PHI for uniform branch: " << MI << '\n');
    break;
  }
}

[cfang]

Merge the test into uniform-cfg.ll.

I can not find an approach to change the test to a kernel and yet still be able to reproduce the issue.
So I think it is OK to use a function here.

[cfang]

Ping! Thanks.

[arsenm]

LGTM

llvm/trunk/lib/Target/AMDGPU/SIFixSGPRCopies.cpp

for (MachineBasicBlock::iterator I = MBB.begin(), E = MBB.end();
       }
       case AMDGPU::PHI: {
         unsigned Reg = MI.getOperand(0).getReg();
         if (!TRI->isSGPRClass(MRI.getRegClass(Reg)))
           break;
 
         // We don't need to fix the PHI if the common dominator of the
         // two incoming blocks terminates with a uniform branch.
-        if (MI.getNumExplicitOperands() == 5) {
+        bool HasVGPROperand = phiHasVGPROperands(MI, MRI, TRI, TII);
+        if (MI.getNumExplicitOperands() == 5 && !HasVGPROperand) {
           MachineBasicBlock *MBB0 = MI.getOperand(2).getMBB();
           MachineBasicBlock *MBB1 = MI.getOperand(4).getMBB();
 
           if (!predsHasDivergentTerminator(MBB0, TRI) &&
               !predsHasDivergentTerminator(MBB1, TRI)) {
             DEBUG(dbgs() << "Not fixing PHI for uniform branch: " << MI << '\n');
             break;
           }
for (MachineBasicBlock::iterator I = MBB.begin(), E = MBB.end();
         // The one exception to this rule is when one of the operands
         // is defined by a SI_BREAK, SI_IF_BREAK, or SI_ELSE_BREAK
         // instruction.  In this case, there we know the program will
         // never enter the second block (the loop) without entering
         // the first block (where the condition is computed), so there
         // is no chance for values to be over-written.
 
         SmallSet<unsigned, 8> Visited;
-        if (phiHasVGPROperands(MI, MRI, TRI, TII) ||
-            !phiHasBreakDef(MI, MRI, Visited)) {
+        if (HasVGPROperand || !phiHasBreakDef(MI, MRI, Visited)) {
           DEBUG(dbgs() << "Fixing PHI: " << MI);
           TII->moveToVALU(MI);
         }
         break;
       }
       case AMDGPU::REG_SEQUENCE: {
         if (TRI->hasVGPRs(TII->getOpRegClass(MI, 0)) ||
             !hasVGPROperands(MI, TRI)) {

llvm/trunk/test/CodeGen/AMDGPU/uniform-cfg.ll

else:
   br label %done
 
 done:
   %value = phi i32 [0, %if], [1, %else]
   store i32 %value, i32 addrspace(1)* %out
   ret void
 }
 
+; GCN-LABEL: {{^}}move_to_valu_vgpr_operand_phi:
+; GCN: v_add_i32_e32
+; GCN: ds_write_b32
+define void @move_to_valu_vgpr_operand_phi(i32 addrspace(3)* %out) {
+bb0:
+  br label %bb1
+
+bb1:                                              ; preds = %bb3, %bb0
+  %tmp0 = phi i32 [ 8, %bb0 ], [ %tmp4, %bb3 ]
+  %tmp1 = add nsw i32 %tmp0, -1
+  %tmp2 = getelementptr inbounds i32, i32 addrspace(3)* %out, i32 %tmp1
+  br i1 undef, label %bb2, label %bb3
+
+bb2:                                              ; preds = %bb1
+  store volatile i32 1, i32 addrspace(3)* %tmp2, align 4
+  br label %bb3
+
+bb3:                                              ; preds = %bb2, %bb1
+  %tmp4 = add nsw i32 %tmp0, 2
+  br label %bb1
+}
+
 declare i32 @llvm.amdgcn.workitem.id.x() #0
 
 attributes #0 = { nounwind readnone }