AMDGPU: Use ReversePostOrder when fixing i1 copies

[arsenm]

lib/Target/AMDGPU/SILowerI1Copies.cpp

697–698

I don't see where the physical register check is coming from. If SrcReg is a virtual register (which it should be), this should crash?

test/CodeGen/AMDGPU/si-lower-i1-copies-vgpr.mir

2 ↗

(On Diff #205445)

This isn't really checking anything. Generated checks with update_mir_test_checks are probably OK

[david-salinas]

original fix was incorrect. This update forces the RegClass if the destination operand is VReg_1, to SReg_32. And then removes the attempt to change the new MI Operand(1).

[arsenm]

Diff looks wrong and is missing test

[nhaehnle]

I'd prefer the alternative fix at D63871, since it doesn't require RPOT.

lib/Target/AMDGPU/SILowerI1Copies.cpp

public:
   void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.setPreservesCFG();
     AU.addRequired<MachineDominatorTree>();
     AU.addRequired<MachinePostDominatorTree>();
     MachineFunctionPass::getAnalysisUsage(AU);
   }
 
 private:
-  void lowerCopiesFromI1();
-  void lowerPhis();
-  void lowerCopiesToI1();
+  void lowerCopiesFromI1(ReversePostOrderTraversal<MachineFunction *> &);
+  void lowerPhis(ReversePostOrderTraversal<MachineFunction *> &);
+  void lowerCopiesToI1(ReversePostOrderTraversal<MachineFunction *> &);
   bool isConstantLaneMask(unsigned Reg, bool &Val) const;
   void buildMergeLaneMasks(MachineBasicBlock &MBB,
                            MachineBasicBlock::iterator I, const DebugLoc &DL,
                            unsigned DstReg, unsigned PrevReg, unsigned CurReg);
   MachineBasicBlock::iterator
   getSaluInsertionAtEnd(MachineBasicBlock &MBB) const;
 
   bool isLaneMaskReg(unsigned Reg) const {
if (IsWave32) {
     MovOp = AMDGPU::S_MOV_B64;
     AndOp = AMDGPU::S_AND_B64;
     OrOp = AMDGPU::S_OR_B64;
     XorOp = AMDGPU::S_XOR_B64;
     AndN2Op = AMDGPU::S_ANDN2_B64;
     OrN2Op = AMDGPU::S_ORN2_B64;
   }
 
-  lowerCopiesFromI1();
-  lowerPhis();
-  lowerCopiesToI1();
+  ReversePostOrderTraversal<MachineFunction *> RPOT(&TheMF);
+
+  lowerCopiesFromI1(RPOT);
+  lowerPhis(RPOT);
+  lowerCopiesToI1(RPOT);
 
   for (unsigned Reg : ConstrainRegs)
     MRI->constrainRegClass(Reg, &AMDGPU::SReg_1_XEXECRegClass);
   ConstrainRegs.clear();
 
   return true;
 }
 
-void SILowerI1Copies::lowerCopiesFromI1() {
+void SILowerI1Copies::lowerCopiesFromI1(
+    ReversePostOrderTraversal<MachineFunction *> &RPOT) {
   SmallVector<MachineInstr *, 4> DeadCopies;
 
-  for (MachineBasicBlock &MBB : *MF) {
-    for (MachineInstr &MI : MBB) {
+  for (MachineBasicBlock *MBB : RPOT) {
+    for (MachineInstr &MI : *MBB) {
       if (MI.getOpcode() != AMDGPU::COPY)
         continue;
 
       unsigned DstReg = MI.getOperand(0).getReg();
       unsigned SrcReg = MI.getOperand(1).getReg();
       if (!TargetRegisterInfo::isVirtualRegister(SrcReg) ||
           MRI->getRegClass(SrcReg) != &AMDGPU::VReg_1RegClass)
         continue;
 
       if (isLaneMaskReg(DstReg) ||
           (TargetRegisterInfo::isVirtualRegister(DstReg) &&
            MRI->getRegClass(DstReg) == &AMDGPU::VReg_1RegClass))
         continue;
 
       // Copy into a 32-bit vector register.
       LLVM_DEBUG(dbgs() << "Lower copy from i1: " << MI);
       DebugLoc DL = MI.getDebugLoc();
 
       assert(TII->getRegisterInfo().getRegSizeInBits(DstReg, *MRI) == 32);
       assert(!MI.getOperand(0).getSubReg());
 
       ConstrainRegs.insert(SrcReg);
-      BuildMI(MBB, MI, DL, TII->get(AMDGPU::V_CNDMASK_B32_e64), DstReg)
+      BuildMI(*MBB, MI, DL, TII->get(AMDGPU::V_CNDMASK_B32_e64), DstReg)
           .addImm(0)
           .addImm(0)
           .addImm(0)
           .addImm(-1)
           .addReg(SrcReg);
       DeadCopies.push_back(&MI);
     }
 
     for (MachineInstr *MI : DeadCopies)
       MI->eraseFromParent();
     DeadCopies.clear();
   }
 }
 
-void SILowerI1Copies::lowerPhis() {
+void SILowerI1Copies::lowerPhis(
+    ReversePostOrderTraversal<MachineFunction *> &RPOT) {
   MachineSSAUpdater SSAUpdater(*MF);
   LoopFinder LF(*DT, *PDT);
   PhiIncomingAnalysis PIA(*PDT);
   SmallVector<MachineInstr *, 4> DeadPhis;
   SmallVector<MachineBasicBlock *, 4> IncomingBlocks;
   SmallVector<unsigned, 4> IncomingRegs;
   SmallVector<unsigned, 4> IncomingUpdated;
 #ifndef NDEBUG
   DenseSet<unsigned> PhiRegisters;
 #endif
 
-  for (MachineBasicBlock &MBB : *MF) {
-    LF.initialize(MBB);
+  for (MachineBasicBlock *MBB : RPOT) {
+    LF.initialize(*MBB);
 
-    for (MachineInstr &MI : MBB.phis()) {
+    for (MachineInstr &MI : MBB->phis()) {
       unsigned DstReg = MI.getOperand(0).getReg();
       if (MRI->getRegClass(DstReg) != &AMDGPU::VReg_1RegClass)
         continue;
 
       LLVM_DEBUG(dbgs() << "Lower PHI: " << MI);
 
       MRI->setRegClass(DstReg, IsWave32 ? &AMDGPU::SReg_32RegClass
                                         : &AMDGPU::SReg_64RegClass);
for (MachineInstr &MI : MBB->phis()) {
       }
 
 #ifndef NDEBUG
       PhiRegisters.insert(DstReg);
 #endif
 
       // Phis in a loop that are observed outside the loop receive a simple but
       // conservatively correct treatment.
-      MachineBasicBlock *PostDomBound = &MBB;
+      MachineBasicBlock *PostDomBound = MBB;
       for (MachineInstr &Use : MRI->use_instructions(DstReg)) {
         PostDomBound =
             PDT->findNearestCommonDominator(PostDomBound, Use.getParent());
       }
 
       unsigned FoundLoopLevel = LF.findLoop(PostDomBound);
 
       SSAUpdater.Initialize(DstReg);
#endif
           MachineBasicBlock &IMBB = *IncomingBlocks[i];
           buildMergeLaneMasks(
               IMBB, getSaluInsertionAtEnd(IMBB), {}, IncomingUpdated[i],
               SSAUpdater.GetValueInMiddleOfBlock(&IMBB), IncomingRegs[i]);
         }
       } else {
         // The phi is not observed from outside a loop. Use a more accurate
         // lowering.
-        PIA.analyze(MBB, IncomingBlocks);
+        PIA.analyze(*MBB, IncomingBlocks);
 
         for (MachineBasicBlock *MBB : PIA.predecessors())
           SSAUpdater.AddAvailableValue(MBB, insertUndefLaneMask(*MBB));
 
         for (unsigned i = 0; i < IncomingRegs.size(); ++i) {
           MachineBasicBlock &IMBB = *IncomingBlocks[i];
           if (PIA.isSource(IMBB)) {
             IncomingUpdated.push_back(0);
#endif
 
           MachineBasicBlock &IMBB = *IncomingBlocks[i];
           buildMergeLaneMasks(
               IMBB, getSaluInsertionAtEnd(IMBB), {}, IncomingUpdated[i],
               SSAUpdater.GetValueInMiddleOfBlock(&IMBB), IncomingRegs[i]);
         }
       }
 
-      unsigned NewReg = SSAUpdater.GetValueInMiddleOfBlock(&MBB);
+      unsigned NewReg = SSAUpdater.GetValueInMiddleOfBlock(MBB);
       if (NewReg != DstReg) {
         MRI->replaceRegWith(NewReg, DstReg);
 
         // Ensure that DstReg has a single def and mark the old PHI node for
         // deletion.
         MI.getOperand(0).setReg(NewReg);
         DeadPhis.push_back(&MI);
       }
 
       IncomingBlocks.clear();
       IncomingRegs.clear();
       IncomingUpdated.clear();
     }
 
     for (MachineInstr *MI : DeadPhis)
       MI->eraseFromParent();
     DeadPhis.clear();
   }
 }
 
-void SILowerI1Copies::lowerCopiesToI1() {
+void SILowerI1Copies::lowerCopiesToI1(
+    ReversePostOrderTraversal<MachineFunction *> &RPOT) {
   MachineSSAUpdater SSAUpdater(*MF);
   LoopFinder LF(*DT, *PDT);
   SmallVector<MachineInstr *, 4> DeadCopies;
 
-  for (MachineBasicBlock &MBB : *MF) {
-    LF.initialize(MBB);
+  for (MachineBasicBlock *MBB : RPOT) {
+    LF.initialize(*MBB);
 
-    for (MachineInstr &MI : MBB) {
+    for (MachineInstr &MI : *MBB) {
       if (MI.getOpcode() != AMDGPU::IMPLICIT_DEF &&
           MI.getOpcode() != AMDGPU::COPY)
         continue;
 
       unsigned DstReg = MI.getOperand(0).getReg();
       if (!TargetRegisterInfo::isVirtualRegister(DstReg) ||
           MRI->getRegClass(DstReg) != &AMDGPU::VReg_1RegClass)
         continue;
for (MachineInstr &MI : *MBB) {
       if (MI.getOpcode() == AMDGPU::IMPLICIT_DEF)
         continue;
 
       DebugLoc DL = MI.getDebugLoc();
       unsigned SrcReg = MI.getOperand(1).getReg();
       assert(!MI.getOperand(1).getSubReg());
 
       if (!TargetRegisterInfo::isVirtualRegister(SrcReg) ||
           !isLaneMaskReg(SrcReg)) {
         assert(TII->getRegisterInfo().getRegSizeInBits(SrcReg, *MRI) == 32);
         unsigned TmpReg = createLaneMaskReg(*MF);
-        BuildMI(MBB, MI, DL, TII->get(AMDGPU::V_CMP_NE_U32_e64), TmpReg)
+        BuildMI(*MBB, MI, DL, TII->get(AMDGPU::V_CMP_NE_U32_e64), TmpReg)
             .addReg(SrcReg)
             .addImm(0);
         MI.getOperand(1).setReg(TmpReg);
         SrcReg = TmpReg;
       }
 
       // Defs in a loop that are observed outside the loop must be transformed
       // into appropriate bit manipulation.
-      MachineBasicBlock *PostDomBound = &MBB;
+      MachineBasicBlock *PostDomBound = MBB;
       for (MachineInstr &Use : MRI->use_instructions(DstReg)) {
         PostDomBound =
             PDT->findNearestCommonDominator(PostDomBound, Use.getParent());
       }
 
       unsigned FoundLoopLevel = LF.findLoop(PostDomBound);
       if (FoundLoopLevel) {
         SSAUpdater.Initialize(DstReg);
-        SSAUpdater.AddAvailableValue(&MBB, DstReg);
+        SSAUpdater.AddAvailableValue(MBB, DstReg);
         LF.addLoopEntries(FoundLoopLevel, SSAUpdater);
 
-        buildMergeLaneMasks(MBB, MI, DL, DstReg,
-                            SSAUpdater.GetValueInMiddleOfBlock(&MBB), SrcReg);
+        buildMergeLaneMasks(*MBB, MI, DL, DstReg,
+                            SSAUpdater.GetValueInMiddleOfBlock(MBB), SrcReg);
         DeadCopies.push_back(&MI);
       }
     }
 
     for (MachineInstr *MI : DeadCopies)
       MI->eraseFromParent();
     DeadCopies.clear();
   }

test/CodeGen/AMDGPU/i1-copies-rpo.mir

+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -mtriple=amdgcn-amd-amdhsa -verify-machineinstrs  -run-pass=si-i1-copies -o - %s | FileCheck %s
+
+# The strange block ordering visits the use before the def.
+---
+name: inserted_cmp_operand_class_rpo
+tracksRegLiveness: true
+machineFunctionInfo:
+  isEntryFunction: true
+body:             |
+  ; CHECK-LABEL: name: inserted_cmp_operand_class_rpo
+  ; CHECK: bb.0:
+  ; CHECK:   successors: %bb.3(0x80000000)
+  ; CHECK:   S_BRANCH %bb.3
+  ; CHECK: bb.1:
+  ; CHECK:   successors: %bb.2(0x80000000)
+  ; CHECK:   [[COPY:%[0-9]+]]:sreg_64 = COPY %1
+  ; CHECK: bb.2:
+  ; CHECK:   [[V_MOV_B32_e32_:%[0-9]+]]:vgpr_32 = V_MOV_B32_e32 0, implicit $exec
+  ; CHECK:   [[COPY1:%[0-9]+]]:sreg_64_xexec = COPY [[COPY]]
+  ; CHECK:   S_ENDPGM 0
+  ; CHECK: bb.3:
+  ; CHECK:   successors: %bb.1(0x80000000)
+  ; CHECK:   [[V_MOV_B32_e32_1:%[0-9]+]]:vgpr_32 = V_MOV_B32_e32 0, implicit $exec
+  ; CHECK:   [[S_MOV_B32_:%[0-9]+]]:sreg_32_xm0 = S_MOV_B32 0
+  ; CHECK:   [[V_CMP_EQ_U32_e64_:%[0-9]+]]:sreg_64 = V_CMP_EQ_U32_e64 killed [[V_MOV_B32_e32_1]], killed [[S_MOV_B32_]], implicit $exec
+  ; CHECK:   [[COPY2:%[0-9]+]]:sreg_64 = COPY [[V_CMP_EQ_U32_e64_]]
+  ; CHECK:   S_BRANCH %bb.1
+  bb.0:
+    successors: %bb.3
+
+    S_BRANCH %bb.3
+
+  bb.1:
+    successors: %bb.2
+
+    %0:vreg_1 = COPY %1
+
+  bb.2:
+    %2:vgpr_32 = V_MOV_B32_e32 0, implicit $exec
+    %3:sreg_64_xexec = COPY %0
+    S_ENDPGM 0
+
+  bb.3:
+    successors: %bb.1
+
+    %4:vgpr_32 = V_MOV_B32_e32 0, implicit $exec
+    %5:sreg_32_xm0 = S_MOV_B32 0
+    %6:sreg_64 = V_CMP_EQ_U32_e64 killed %4, killed %5, implicit $exec
+    %1:vreg_1 = COPY %6
+    S_BRANCH %bb.1