[GlobalISel] Fix insertion point of new instructions to be after PHIs

[aemerson]

Summary

For some reason we sometimes insert new instructions one instruction before the first non-PHI when legalizing. This can result in having non-PHI instructions before PHIs, which mean that PHI elimination doesn't catch them.

[volkan]

LGTM.

llvm/trunk/lib/CodeGen/GlobalISel/LegalizerHelper.cpp

case TargetOpcode::G_PHI: {
     MIRBuilder.setInsertPt(MBB, MI);
     for (unsigned i = 0; i < NumParts; ++i) {
       DstRegs[i] = MRI.createGenericVirtualRegister(NarrowTy);
       MachineInstrBuilder MIB =
           MIRBuilder.buildInstr(TargetOpcode::G_PHI).addDef(DstRegs[i]);
       for (unsigned j = 1; j < MI.getNumOperands(); j += 2)
         MIB.addUse(SrcRegs[j / 2][i]).add(MI.getOperand(j + 1));
     }
-    MIRBuilder.setInsertPt(MBB, --MBB.getFirstNonPHI());
+    MIRBuilder.setInsertPt(MBB, MBB.getFirstNonPHI());
     MIRBuilder.buildMerge(MI.getOperand(0).getReg(), DstRegs);
     Observer.changedInstr(MI);
     MI.eraseFromParent();
     return Legalized;
   }
   case TargetOpcode::G_EXTRACT_VECTOR_ELT:
   case TargetOpcode::G_INSERT_VECTOR_ELT: {
     if (TypeIdx != 2)
case TargetOpcode::G_PHI: {
     Observer.changingInstr(MI);
     for (unsigned I = 1; I < MI.getNumOperands(); I += 2) {
       MachineBasicBlock &OpMBB = *MI.getOperand(I + 1).getMBB();
       MIRBuilder.setInsertPt(OpMBB, OpMBB.getFirstTerminator());
       widenScalarSrc(MI, WideTy, I, TargetOpcode::G_ANYEXT);
     }
 
     MachineBasicBlock &MBB = *MI.getParent();
-    MIRBuilder.setInsertPt(MBB, --MBB.getFirstNonPHI());
+    MIRBuilder.setInsertPt(MBB, MBB.getFirstNonPHI());
     widenScalarDst(MI, WideTy);
     Observer.changedInstr(MI);
     return Legalized;
   }
   case TargetOpcode::G_EXTRACT_VECTOR_ELT: {
     if (TypeIdx == 0) {
       Register VecReg = MI.getOperand(1).getReg();
       LLT VecTy = MRI.getType(VecReg);
LegalizerHelper::moreElementsVectorPhi(MachineInstr &MI, unsigned TypeIdx,
   Observer.changingInstr(MI);
   for (unsigned I = 1, E = MI.getNumOperands(); I != E; I += 2) {
     MachineBasicBlock &OpMBB = *MI.getOperand(I + 1).getMBB();
     MIRBuilder.setInsertPt(OpMBB, OpMBB.getFirstTerminator());
     moreElementsVectorSrc(MI, MoreTy, I);
   }
 
   MachineBasicBlock &MBB = *MI.getParent();
-  MIRBuilder.setInsertPt(MBB, --MBB.getFirstNonPHI());
+  MIRBuilder.setInsertPt(MBB, MBB.getFirstNonPHI());
   moreElementsVectorDst(MI, MoreTy, 0);
   Observer.changedInstr(MI);
   return Legalized;
 }
 
 LegalizerHelper::LegalizeResult
 LegalizerHelper::moreElementsVector(MachineInstr &MI, unsigned TypeIdx,
                                     LLT MoreTy) {

llvm/trunk/test/CodeGen/AArch64/GlobalISel/legalize-phi.mir

entry:
     ret i32 0
   }
 
   define i32 @legalize_phi_diff_bb(i32 %argc, i32 %argc2) {
   entry:
     ret i32 0
   }
 
+  define i32 @legalize_phi_check_insertpt(i64 %a) {
+  entry:
+    ret i32 0
+  }
+
 ...
 ---
 name:            legalize_phi
 alignment:       4
 exposesReturnsTwice: false
 legalized:       false
 regBankSelected: false
 selected:        false
body:             |
 
   bb.3:
     %13(s8) = G_PHI %7(s8), %bb.1, %6(s8), %bb.0
     %14(s32) = G_ZEXT %13(s8)
     $w0 = COPY %14(s32)
     RET_ReallyLR implicit $w0
 
 ...
+---
+name:            legalize_phi_check_insertpt
+alignment:       4
+exposesReturnsTwice: false
+legalized:       false
+regBankSelected: false
+selected:        false
+tracksRegLiveness: true
+body:             |
+  ; Check that the G_MERGE here gets inserted after all the PHIs.
+  ; CHECK-LABEL: name: legalize_phi_check_insertpt
+  ; CHECK: bb.0:
+  ; CHECK:   successors: %bb.1(0x80000000)
+  ; CHECK:   liveins: $x0, $x1
+  ; CHECK:   [[COPY:%[0-9]+]]:_(s64) = COPY $x0
+  ; CHECK:   [[COPY1:%[0-9]+]]:_(p0) = COPY $x1
+  ; CHECK:   [[DEF:%[0-9]+]]:_(s64) = G_IMPLICIT_DEF
+  ; CHECK:   [[DEF1:%[0-9]+]]:_(s64) = G_IMPLICIT_DEF
+  ; CHECK:   G_BR %bb.1
+  ; CHECK: bb.1:
+  ; CHECK:   [[PHI:%[0-9]+]]:_(s64) = G_PHI [[DEF]](s64), %bb.0
+  ; CHECK:   [[PHI1:%[0-9]+]]:_(s64) = G_PHI [[DEF1]](s64), %bb.0
+  ; CHECK:   [[PHI2:%[0-9]+]]:_(s64) = G_PHI [[COPY]](s64), %bb.0
+  ; CHECK:   [[MV:%[0-9]+]]:_(s128) = G_MERGE_VALUES [[PHI]](s64), [[PHI1]](s64)
+  ; CHECK:   G_STORE [[MV]](s128), [[COPY1]](p0) :: (store 16)
+  ; CHECK:   G_STORE [[PHI2]](s64), [[COPY1]](p0) :: (store 8)
+  ; CHECK:   RET_ReallyLR
+  bb.0:
+    successors: %bb.1(0x40000000)
+    liveins: $x0, $x1
+
+    %0:_(s64) = COPY $x0
+    %1:_(p0) = COPY $x1
+    %2:_(s128) = G_IMPLICIT_DEF
+    G_BR %bb.1
+
+  bb.1:
+    %3:_(s128) = G_PHI %2(s128), %bb.0
+    %4:_(s64) = G_PHI %0(s64), %bb.0
+    G_STORE %3(s128), %1(p0) :: (store 16)
+    G_STORE %4(s64), %1(p0) :: (store 8)
+    RET_ReallyLR
+
+...

llvm/trunk/test/CodeGen/AMDGPU/GlobalISel/legalize-phi.mir

body: |
   ; CHECK:   [[COPY10:%[0-9]+]]:_(s32) = COPY [[ADD2]](s32)
   ; CHECK:   [[BUILD_VECTOR:%[0-9]+]]:_(<3 x s32>) = G_BUILD_VECTOR [[COPY8]](s32), [[COPY9]](s32), [[COPY10]](s32)
   ; CHECK:   [[TRUNC:%[0-9]+]]:_(<3 x s16>) = G_TRUNC [[BUILD_VECTOR]](<3 x s32>)
   ; CHECK:   [[DEF3:%[0-9]+]]:_(<4 x s16>) = G_IMPLICIT_DEF
   ; CHECK:   [[INSERT3:%[0-9]+]]:_(<4 x s16>) = G_INSERT [[DEF3]], [[TRUNC]](<3 x s16>), 0
   ; CHECK:   G_BR %bb.2
   ; CHECK: bb.2:
   ; CHECK:   [[PHI:%[0-9]+]]:_(<4 x s16>) = G_PHI [[INSERT]](<4 x s16>), %bb.0, [[INSERT3]](<4 x s16>), %bb.1
-  ; CHECK:   [[EXTRACT1:%[0-9]+]]:_(<3 x s16>) = G_EXTRACT [[PHI]](<4 x s16>), 0
   ; CHECK:   [[DEF4:%[0-9]+]]:_(<4 x s16>) = G_IMPLICIT_DEF
+  ; CHECK:   [[EXTRACT1:%[0-9]+]]:_(<3 x s16>) = G_EXTRACT [[PHI]](<4 x s16>), 0
   ; CHECK:   [[INSERT4:%[0-9]+]]:_(<4 x s16>) = G_INSERT [[DEF4]], [[EXTRACT1]](<3 x s16>), 0
   ; CHECK:   $vgpr0_vgpr1 = COPY [[INSERT4]](<4 x s16>)
   ; CHECK:   S_SETPC_B64 undef $sgpr30_sgpr31
   bb.0:
     successors: %bb.1, %bb.2
     liveins: $vgpr0_vgpr1, $vgpr2
 
     %0:_(<4 x s16>) = COPY $vgpr0_vgpr1