Tweak the fix for PR3784: be less sensitive about just

how invokes are set up.  The fix could be disturbed by
register copies coming after the EH_LABEL, and also didn't
behave quite right when it was the invoke result that
was used in a phi node.  Also (see new testcase) fix
another phi elimination bug while there: register copies
in the landing pad need to come after the EH_LABEL, because
that's where execution branches to when unwinding.  If they
come before the EH_LABEL then they will never be executed...
Also tweak the original testcase so it doesn't use a no-longer
existing counter.
The accumulated phi elimination changes fix two of seven Ada
testsuite failures that turned up after landing pad critical
edge splitting was turned off.  So there's probably more to come.

llvm-svn: 67049

lib/CodeGen/PHIElimination.cpp

@@ -14,6 +14,8 @@
 //===----------------------------------------------------------------------===//
 
 #define DEBUG_TYPE "phielim"
+#include "llvm/BasicBlock.h"
+#include "llvm/Instructions.h"
 #include "llvm/CodeGen/LiveVariables.h"
 #include "llvm/CodeGen/Passes.h"
 #include "llvm/CodeGen/MachineFunctionPass.h"
@@ -31,7 +33,6 @@
 using namespace llvm;
 
 STATISTIC(NumAtomic, "Number of atomic phis lowered");
-STATISTIC(NumEH,     "Number of EH try blocks skipped");
 
 namespace {
   class VISIBILITY_HIDDEN PNE : public MachineFunctionPass {
@@ -66,8 +67,25 @@ namespace {
     ///
     void analyzePHINodes(const MachineFunction& Fn);
 
-    void WalkPassEHTryRange(MachineBasicBlock &MBB,
-                            MachineBasicBlock::iterator &I, unsigned SrcReg);
+    // FindCopyInsertPoint - Find a safe place in MBB to insert a copy from
+    // SrcReg.  This needs to be after any def or uses of SrcReg, but before
+    // any subsequent point where control flow might jump out of the basic
+    // block.
+    MachineBasicBlock::iterator FindCopyInsertPoint(MachineBasicBlock &MBB,
+                                                    unsigned SrcReg);
+
+    // SkipPHIsAndLabels - Copies need to be inserted after phi nodes and
+    // also after any exception handling labels: in landing pads execution
+    // starts at the label, so any copies placed before it won't be executed!
+    MachineBasicBlock::iterator SkipPHIsAndLabels(MachineBasicBlock &MBB,
+                                                MachineBasicBlock::iterator I) {
+      // Rather than assuming that EH labels come before other kinds of labels,
+      // just skip all labels.
+      while (I != MBB.end() &&
+             (I->getOpcode() == TargetInstrInfo::PHI || I->isLabel()))
+        ++I;
+      return I;
+    }
 
     typedef std::pair<const MachineBasicBlock*, unsigned> BBVRegPair;
     typedef std::map<BBVRegPair, unsigned> VRegPHIUse;
@@ -120,10 +138,7 @@ bool PNE::EliminatePHINodes(MachineFunction &MF, MachineBasicBlock &MBB) {
 
   // Get an iterator to the first instruction after the last PHI node (this may
   // also be the end of the basic block).
-  MachineBasicBlock::iterator AfterPHIsIt = MBB.begin();
-  while (AfterPHIsIt != MBB.end() &&
-         AfterPHIsIt->getOpcode() == TargetInstrInfo::PHI)
-    ++AfterPHIsIt;    // Skip over all of the PHI nodes...
+  MachineBasicBlock::iterator AfterPHIsIt = SkipPHIsAndLabels(MBB, MBB.begin());
 
   while (MBB.front().getOpcode() == TargetInstrInfo::PHI)
     LowerAtomicPHINode(MBB, AfterPHIsIt);
@@ -144,37 +159,47 @@ static bool isSourceDefinedByImplicitDef(const MachineInstr *MPhi,
   return true;
 }
 
-void PNE::WalkPassEHTryRange(MachineBasicBlock &MBB,
-                             MachineBasicBlock::iterator &I, unsigned SrcReg) {
-  if (I == MBB.begin())
-    return;
-  MachineBasicBlock::iterator PI = prior(I);
-  if (PI->getOpcode() != TargetInstrInfo::EH_LABEL)
-    return;
+// FindCopyInsertPoint - Find a safe place in MBB to insert a copy from SrcReg.
+// This needs to be after any def or uses of SrcReg, but before any subsequent
+// point where control flow might jump out of the basic block.
+MachineBasicBlock::iterator PNE::FindCopyInsertPoint(MachineBasicBlock &MBB,
+                                                     unsigned SrcReg) {
+  // Handle the trivial case trivially.
+  if (MBB.empty())
+    return MBB.begin();
 
-  // Trying to walk pass the EH try range. If we run into a use instruction,
-  // we want to insert the copy there.
-  SmallPtrSet<MachineInstr*, 4> UsesInMBB;
-  for (MachineRegisterInfo::use_iterator UI = MRI->use_begin(SrcReg),
-         UE = MRI->use_end(); UI != UE; ++UI) {
-    MachineInstr *UseMI = &*UI;
-    if (UseMI->getParent() == &MBB)
-      UsesInMBB.insert(UseMI);
+  // If this basic block does not contain an invoke, then control flow always
+  // reaches the end of it, so place the copy there.  The logic below works in
+  // this case too, but is more expensive.
+  if (!isa<InvokeInst>(MBB.getBasicBlock()->getTerminator()))
+    return MBB.getFirstTerminator();
+
+  // Discover any definition/uses in this basic block.
+  SmallPtrSet<MachineInstr*, 8> DefUsesInMBB;
+  for (MachineRegisterInfo::reg_iterator RI = MRI->reg_begin(SrcReg),
+       RE = MRI->reg_end(); RI != RE; ++RI) {
+    MachineInstr *DefUseMI = &*RI;
+    if (DefUseMI->getParent() == &MBB)
+      DefUsesInMBB.insert(DefUseMI);
   }
 
-  while (PI != MBB.begin()) {
-    --PI;
-    if (PI->getOpcode() == TargetInstrInfo::EH_LABEL) {
-      ++NumEH;
-      I = PI;
-      return;
-    } else if (UsesInMBB.count(&*PI)) {
-      ++NumEH;
-      I = next(PI);
-      return;
-    }
+  MachineBasicBlock::iterator InsertPoint;
+  if (DefUsesInMBB.empty()) {
+    // No def/uses.  Insert the copy at the start of the basic block.
+    InsertPoint = MBB.begin();
+  } else if (DefUsesInMBB.size() == 1) {
+    // Insert the copy immediately after the definition/use.
+    InsertPoint = *DefUsesInMBB.begin();
+    ++InsertPoint;
+  } else {
+    // Insert the copy immediately after the last definition/use.
+    InsertPoint = MBB.end();
+    while (!DefUsesInMBB.count(&*--InsertPoint)) {}
+    ++InsertPoint;
   }
-  return;
+
+  // Make sure the copy goes after any phi nodes however.
+  return SkipPHIsAndLabels(MBB, InsertPoint);
 }
 
 /// LowerAtomicPHINode - Lower the PHI node at the top of the specified block,
@@ -273,10 +298,7 @@ void PNE::LowerAtomicPHINode(MachineBasicBlock &MBB,
  
     // Find a safe location to insert the copy, this may be the first terminator
     // in the block (or end()).
-    MachineBasicBlock::iterator InsertPos = opBlock.getFirstTerminator();
-
-    // Walk pass EH try range if needed.
-    WalkPassEHTryRange(opBlock, InsertPos, SrcReg);
+    MachineBasicBlock::iterator InsertPos = FindCopyInsertPoint(opBlock, SrcReg);
 
     // Insert the copy.
     TII->copyRegToReg(opBlock, InsertPos, IncomingReg, SrcReg, RC, RC);

test/CodeGen/X86/2009-03-13-PHIElimBug.ll

@@ -1,37 +1,28 @@
-; RUN: llvm-as < %s | llc -mtriple=i386-pc-linux-gnu -stats |& grep phielim | grep {Number of EH try blocks skipped} | grep 4
+; RUN: llvm-as < %s | llc -march=x86 | grep -A 2 {call	f} | grep movl
+; Check the register copy comes after the call to f and before the call to g
 ; PR3784
 
-	%struct.c38002a__arr___XUB = type { i32, i32 }
-	%struct.c38002a__arr_name = type { [0 x i32]*, %struct.c38002a__arr___XUB* }
-	%struct.c38002a__rec = type { i32, %struct.c38002a__arr_name }
+declare i32 @f()
 
-define void @_ada_c38002a() {
+declare i32 @g()
+
+define i32 @phi() {
 entry:
-	%0 = invoke i8* @__gnat_malloc(i32 12)
-			to label %invcont unwind label %lpad		; <i8*> [#uses=0]
+	%a = call i32 @f()		; <i32> [#uses=1]
+	%b = invoke i32 @g()
+			to label %cont unwind label %lpad		; <i32> [#uses=1]
 
-invcont:		; preds = %entry
-	%1 = invoke i8* @__gnat_malloc(i32 20)
-			to label %invcont1 unwind label %lpad		; <i8*> [#uses=0]
+cont:		; preds = %entry
+	%x = phi i32 [ %b, %entry ]		; <i32> [#uses=0]
+	%aa = call i32 @g()		; <i32> [#uses=1]
+	%bb = invoke i32 @g()
+			to label %cont2 unwind label %lpad		; <i32> [#uses=1]
 
-invcont1:		; preds = %invcont
-	%2 = invoke i32 @report__ident_int(i32 2)
-			to label %.noexc unwind label %lpad		; <i32> [#uses=0]
+cont2:		; preds = %cont
+	%xx = phi i32 [ %bb, %cont ]		; <i32> [#uses=1]
+	ret i32 %xx
 
-.noexc:		; preds = %invcont1
-	%3 = invoke i32 @report__ident_int(i32 3)
-			to label %.noexc88 unwind label %lpad		; <i32> [#uses=0]
-
-.noexc88:		; preds = %.noexc
-	unreachable
-
-lpad:		; preds = %.noexc, %invcont1, %invcont, %entry
-	%r.0 = phi %struct.c38002a__rec* [ null, %entry ], [ null, %invcont ], [ null, %invcont1 ], [ null, %.noexc ]		; <%struct.c38002a__rec*> [#uses=1]
-	%4 = getelementptr %struct.c38002a__rec* %r.0, i32 0, i32 0		; <i32*> [#uses=1]
-	%5 = load i32* %4, align 4		; <i32> [#uses=0]
-	ret void
+lpad:		; preds = %cont, %entry
+	%y = phi i32 [ %a, %entry ], [ %aa, %cont ]		; <i32> [#uses=1]
+	ret i32 %y
 }
-
-declare i32 @report__ident_int(i32)
-
-declare i8* @__gnat_malloc(i32)

test/CodeGen/X86/2009-03-16-PHIElimInLPad.ll

@@ -0,0 +1,21 @@
+; RUN: llvm-as < %s | llc -march=x86 | grep -A 1 lpad | grep Llabel
+; Check that register copies in the landing pad come after the EH_LABEL
+
+declare i32 @f()
+
+define i32 @phi(i32 %x) {
+entry:
+	%a = invoke i32 @f()
+			to label %cont unwind label %lpad		; <i32> [#uses=1]
+
+cont:		; preds = %entry
+	%b = invoke i32 @f()
+			to label %cont2 unwind label %lpad		; <i32> [#uses=1]
+
+cont2:		; preds = %cont
+	ret i32 %b
+
+lpad:		; preds = %cont, %entry
+	%v = phi i32 [ %x, %entry ], [ %a, %cont ]		; <i32> [#uses=1]
+	ret i32 %v
+}