CodeGen: Remove dead code in TailDuplicate
authorJustin Bogner <mail@justinbogner.com>
Mon, 4 Apr 2016 21:11:40 +0000 (21:11 +0000)
committerJustin Bogner <mail@justinbogner.com>
Mon, 4 Apr 2016 21:11:40 +0000 (21:11 +0000)
I noticed that this isn't covered by our existing tests and spent some
time trying to come up with an example it actually hits. I tried hand
rolling something based on the explanation in the comment, but couldn't
get anything that didn't abort tail duplication earlier for one reason
or another.

Then, I tried cranking tail-dup-size cranked up so this would fire
more and ran a bootstrap of clang and the nightly test suite - those
don't hit this either.

This reverts r132816 and replaces it with an assert.

llvm-svn: 265347

llvm/lib/CodeGen/TailDuplication.cpp

index 2a8431c..7f135be 100644 (file)
@@ -92,8 +92,7 @@ namespace {
                     MachineBasicBlock *PredBB,
                     DenseMap<unsigned, unsigned> &LocalVRMap,
                     SmallVectorImpl<std::pair<unsigned,unsigned> > &Copies,
-                    const DenseSet<unsigned> &UsedByPhi,
-                    bool Remove);
+                    const DenseSet<unsigned> &UsedByPhi);
     void DuplicateInstruction(MachineInstr *MI,
                               MachineBasicBlock *TailBB,
                               MachineBasicBlock *PredBB,
@@ -395,7 +394,7 @@ void TailDuplicatePass::ProcessPHI(
     MachineInstr *MI, MachineBasicBlock *TailBB, MachineBasicBlock *PredBB,
     DenseMap<unsigned, unsigned> &LocalVRMap,
     SmallVectorImpl<std::pair<unsigned, unsigned> > &Copies,
-    const DenseSet<unsigned> &RegsUsedByPhi, bool Remove) {
+    const DenseSet<unsigned> &RegsUsedByPhi) {
   unsigned DefReg = MI->getOperand(0).getReg();
   unsigned SrcOpIdx = getPHISrcRegOpIdx(MI, PredBB);
   assert(SrcOpIdx && "Unable to find matching PHI source?");
@@ -410,9 +409,6 @@ void TailDuplicatePass::ProcessPHI(
   if (isDefLiveOut(DefReg, TailBB, MRI) || RegsUsedByPhi.count(DefReg))
     AddSSAUpdateEntry(DefReg, NewDef, PredBB);
 
-  if (!Remove)
-    return;
-
   // Remove PredBB from the PHI node.
   MI->RemoveOperand(SrcOpIdx+1);
   MI->RemoveOperand(SrcOpIdx);
@@ -840,7 +836,7 @@ TailDuplicatePass::TailDuplicate(MachineBasicBlock *TailBB,
       if (MI->isPHI()) {
         // Replace the uses of the def of the PHI with the register coming
         // from PredBB.
-        ProcessPHI(MI, TailBB, PredBB, LocalVRMap, CopyInfos, UsedByPhi, true);
+        ProcessPHI(MI, TailBB, PredBB, LocalVRMap, CopyInfos, UsedByPhi);
       } else {
         // Replace def of virtual registers with new registers, and update
         // uses with PHI source register or the new registers.
@@ -894,7 +890,7 @@ TailDuplicatePass::TailDuplicate(MachineBasicBlock *TailBB,
         // Replace the uses of the def of the PHI with the register coming
         // from PredBB.
         MachineInstr *MI = &*I++;
-        ProcessPHI(MI, TailBB, PrevBB, LocalVRMap, CopyInfos, UsedByPhi, true);
+        ProcessPHI(MI, TailBB, PrevBB, LocalVRMap, CopyInfos, UsedByPhi);
         if (MI->getParent())
           MI->eraseFromParent();
       }
@@ -926,56 +922,16 @@ TailDuplicatePass::TailDuplicate(MachineBasicBlock *TailBB,
     Changed = true;
   }
 
-  // If this is after register allocation, there are no phis to fix.
-  if (!PreRegAlloc)
-    return Changed;
-
-  // If we made no changes so far, we are safe.
-  if (!Changed)
-    return Changed;
-
-
-  // Handle the nasty case in that we duplicated a block that is part of a loop
-  // into some but not all of its predecessors. For example:
-  //    1 -> 2 <-> 3                 |
-  //          \                      |
-  //           \---> rest            |
-  // if we duplicate 2 into 1 but not into 3, we end up with
-  // 12 -> 3 <-> 2 -> rest           |
-  //   \             /               |
-  //    \----->-----/                |
-  // If there was a "var = phi(1, 3)" in 2, it has to be ultimately replaced
-  // with a phi in 3 (which now dominates 2).
-  // What we do here is introduce a copy in 3 of the register defined by the
-  // phi, just like when we are duplicating 2 into 3, but we don't copy any
-  // real instructions or remove the 3 -> 2 edge from the phi in 2.
-  for (SmallSetVector<MachineBasicBlock *, 8>::iterator PI = Preds.begin(),
-       PE = Preds.end(); PI != PE; ++PI) {
-    MachineBasicBlock *PredBB = *PI;
-    if (std::find(TDBBs.begin(), TDBBs.end(), PredBB) != TDBBs.end())
-      continue;
-
-    // EH edges
-    if (PredBB->succ_size() != 1)
-      continue;
-
-    DenseMap<unsigned, unsigned> LocalVRMap;
-    SmallVector<std::pair<unsigned,unsigned>, 4> CopyInfos;
-    MachineBasicBlock::iterator I = TailBB->begin();
-    // Process PHI instructions first.
-    while (I != TailBB->end() && I->isPHI()) {
-      // Replace the uses of the def of the PHI with the register coming
-      // from PredBB.
-      MachineInstr *MI = &*I++;
-      ProcessPHI(MI, TailBB, PredBB, LocalVRMap, CopyInfos, UsedByPhi, false);
-    }
-    MachineBasicBlock::iterator Loc = PredBB->getFirstTerminator();
-    for (unsigned i = 0, e = CopyInfos.size(); i != e; ++i) {
-      Copies.push_back(BuildMI(*PredBB, Loc, DebugLoc(),
-                               TII->get(TargetOpcode::COPY),
-                               CopyInfos[i].first).addReg(CopyInfos[i].second));
-    }
-  }
+  // TODO: We shouldn't need this assert. It's here to be defensive about
+  // removing the logic from r132816, which handled an unreachable case.
+  assert((!PreRegAlloc || !Changed ||
+          std::all_of(Preds.begin(), Preds.end(),
+                      [&TDBBs](MachineBasicBlock *PredBB) {
+                        return (std::find(TDBBs.begin(), TDBBs.end(), PredBB) !=
+                                TDBBs.end()) ||
+                               PredBB->succ_size() != 1;
+                      })) &&
+         "loop block duplicated into some but not all predecessors");
 
   return Changed;
 }