[AArch64][GlobalISel] Handle multiple phis in fixupPHIOpBanks

[paquette]

Summary

If we ended up with two phi instructions in a block, and we needed to fix up the banks for the first one, we'd end up inserting our COPY before the second phi.

E.g.

%x = G_PHI ...
%fixup = COPY ...
%y = G_PHI ...

This is invalid MIR, and breaks assumptions made by the register allocator later down the line. With the verifier enabled, it also emits a verification error.

This teaches fixupPHIOpBanks to walk past any phi instructions in the block when emitting the fixup copies.

Here's an example of the crashing code (same as added testcase):
https://godbolt.org/z/h5j1x3o6e

[aemerson]

llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp

6027

Can we simply use getFirstNonPHI() unconditionally?

[paquette]

llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp

6027

No, because the point where we're inserting the COPY may be in a block without phis.

e.g. in test_loop_phi_gpr_to_fpr:

bb.1:
  successors: %bb.2(0x80000000)

  %6:gpr(s32) = G_IMPLICIT_DEF
  %7:gpr(s32) = G_SELECT %0(s1), %6, %6
  %1:gpr(s16) = G_TRUNC %7(s32)

bb.2:

In this case, OpDef is the G_TRUNC. So, we need to insert the copy after it (at the end of the block). So, inserting after the first non-phi instruction isn't correct here.

[aemerson]

Ok, LGTM.

llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp

for (unsigned OpIdx = 1; OpIdx < MI.getNumOperands(); ++OpIdx) {
     if (!MO.isReg())
       continue;
     Register OpReg = MO.getReg();
     const RegisterBank *RB = MRI.getRegBankOrNull(OpReg);
     if (RB != DstRB) {
       // Insert a cross-bank copy.
       auto *OpDef = MRI.getVRegDef(OpReg);
       const LLT &Ty = MRI.getType(OpReg);
-      MIB.setInsertPt(*OpDef->getParent(), std::next(OpDef->getIterator()));
+      MachineBasicBlock &OpDefBB = *OpDef->getParent();
+
+      // Any instruction we insert must appear after all PHIs in the block
+      // for the block to be valid MIR.
+      MachineBasicBlock::iterator InsertPt = std::next(OpDef->getIterator());
+      if (InsertPt != OpDefBB.end() && InsertPt->isPHI())
+        InsertPt = OpDefBB.getFirstNonPHI();
+      MIB.setInsertPt(*OpDef->getParent(), InsertPt);
       auto Copy = MIB.buildCopy(Ty, OpReg);
       MRI.setRegBank(Copy.getReg(0), *DstRB);
       MO.setReg(Copy.getReg(0));
     }
   }
 }
 
 void AArch64InstructionSelector::processPHIs(MachineFunction &MF) {

llvm/test/CodeGen/AArch64/GlobalISel/preselect-process-phis.mir

bb.2:
     successors: %bb.2(0x80000000)
 
     %3:fpr(s16) = G_PHI %5(s16), %bb.2, %1(s16), %bb.1
     %5:fpr(s16) = G_FPTRUNC %8(s32)
     G_STORE %3(s16), %4(p0) :: (store 2 into `half* undef`)
     G_BR %bb.2
 
 ...
+---
+name:            multiple_phis
+alignment:       4
+legalized:       true
+regBankSelected: true
+tracksRegLiveness: true
+body:             |
+
+  ; The copy we insert in bb.4 should appear after all the phi instructions.
+
+  ; CHECK-LABEL: name: multiple_phis
+  ; CHECK: bb.0:
+  ; CHECK:   successors: %bb.1(0x40000000), %bb.5(0x40000000)
+  ; CHECK:   liveins: $w0, $w1, $x2
+  ; CHECK:   %ptr:gpr64sp = COPY $x2
+  ; CHECK:   %cond_1:gpr32 = IMPLICIT_DEF
+  ; CHECK:   %gpr_1:gpr32 = IMPLICIT_DEF
+  ; CHECK:   [[COPY:%[0-9]+]]:fpr32 = COPY %gpr_1
+  ; CHECK:   [[COPY1:%[0-9]+]]:fpr16 = COPY [[COPY]].hsub
+  ; CHECK:   TBNZW %cond_1, 0, %bb.5
+  ; CHECK:   B %bb.1
+  ; CHECK: bb.1:
+  ; CHECK:   successors: %bb.2(0x40000000), %bb.3(0x40000000)
+  ; CHECK:   %cond_2:gpr32 = IMPLICIT_DEF
+  ; CHECK:   TBNZW %cond_2, 0, %bb.3
+  ; CHECK:   B %bb.2
+  ; CHECK: bb.2:
+  ; CHECK:   successors: %bb.4(0x80000000)
+  ; CHECK:   %gpr_2:gpr32 = IMPLICIT_DEF
+  ; CHECK:   B %bb.4
+  ; CHECK: bb.3:
+  ; CHECK:   successors: %bb.4(0x80000000)
+  ; CHECK:   %fpr:fpr16 = IMPLICIT_DEF
+  ; CHECK: bb.4:
+  ; CHECK:   successors: %bb.5(0x80000000)
+  ; CHECK:   %fp_phi:fpr16 = PHI %fpr, %bb.3, [[COPY1]], %bb.2
+  ; CHECK:   %gp_phi1:gpr32 = PHI %gpr_1, %bb.3, %gpr_2, %bb.2
+  ; CHECK:   %gp_phi2:gpr32 = PHI %gpr_1, %bb.3, %gpr_2, %bb.2
+  ; CHECK:   %gp_phi3:gpr32 = PHI %gpr_1, %bb.3, %gpr_2, %bb.2
+  ; CHECK:   [[SUBREG_TO_REG:%[0-9]+]]:fpr32 = SUBREG_TO_REG 0, %fp_phi, %subreg.hsub
+  ; CHECK:   [[COPY2:%[0-9]+]]:gpr32all = COPY [[SUBREG_TO_REG]]
+  ; CHECK: bb.5:
+  ; CHECK:   %use_fp_phi:gpr32 = PHI %gpr_1, %bb.0, [[COPY2]], %bb.4
+  ; CHECK:   %use_gp_phi1:gpr32 = PHI %gpr_1, %bb.0, %gp_phi1, %bb.4
+  ; CHECK:   %use_gp_phi2:gpr32 = PHI %gpr_1, %bb.0, %gp_phi2, %bb.4
+  ; CHECK:   %use_gp_phi3:gpr32 = PHI %gpr_1, %bb.0, %gp_phi3, %bb.4
+  ; CHECK:   STRHHui %use_fp_phi, %ptr, 0 :: (store 2)
+  ; CHECK:   STRHHui %use_gp_phi1, %ptr, 0 :: (store 2)
+  ; CHECK:   STRHHui %use_gp_phi2, %ptr, 0 :: (store 2)
+  ; CHECK:   STRHHui %use_gp_phi3, %ptr, 0 :: (store 2)
+  ; CHECK:   RET_ReallyLR
+  bb.1:
+    successors: %bb.2, %bb.6
+    liveins: $w0, $w1, $x2
+    %ptr:gpr(p0) = COPY $x2
+    %cond_1:gpr(s1) = G_IMPLICIT_DEF
+    %gpr_1:gpr(s16) = G_IMPLICIT_DEF
+    G_BRCOND %cond_1(s1), %bb.6
+    G_BR %bb.2
+  bb.2:
+    successors: %bb.3, %bb.4
+    %cond_2:gpr(s1) = G_IMPLICIT_DEF
+    G_BRCOND %cond_2(s1), %bb.4
+    G_BR %bb.3
+  bb.3:
+    %gpr_2:gpr(s16) = G_IMPLICIT_DEF
+    G_BR %bb.5
+  bb.4:
+    %fpr:fpr(s16) = G_IMPLICIT_DEF
+  bb.5:
+    %fp_phi:fpr(s16) = G_PHI %fpr(s16), %bb.4, %gpr_1(s16), %bb.3
+    %gp_phi1:gpr(s16) = G_PHI %gpr_1(s16), %bb.4, %gpr_2(s16), %bb.3
+    %gp_phi2:gpr(s16) = G_PHI %gpr_1(s16), %bb.4, %gpr_2(s16), %bb.3
+    %gp_phi3:gpr(s16) = G_PHI %gpr_1(s16), %bb.4, %gpr_2(s16), %bb.3
+  bb.6:
+    %use_fp_phi:gpr(s16) = G_PHI %gpr_1(s16), %bb.1, %fp_phi(s16), %bb.5
+    %use_gp_phi1:gpr(s16) = G_PHI %gpr_1(s16), %bb.1, %gp_phi1(s16), %bb.5
+    %use_gp_phi2:gpr(s16) = G_PHI %gpr_1(s16), %bb.1, %gp_phi2(s16), %bb.5
+    %use_gp_phi3:gpr(s16) = G_PHI %gpr_1(s16), %bb.1, %gp_phi3(s16), %bb.5
+    G_STORE %use_fp_phi(s16), %ptr(p0) :: (store 2)
+    G_STORE %use_gp_phi1(s16), %ptr(p0) :: (store 2)
+    G_STORE %use_gp_phi2(s16), %ptr(p0) :: (store 2)
+    G_STORE %use_gp_phi3(s16), %ptr(p0) :: (store 2)
+    RET_ReallyLR
+...