[RewriteStatepointsForGC] Missed review comment from 234651 & build fix
[oota-llvm.git] / lib / Transforms / Scalar / RewriteStatepointsForGC.cpp
index e3859bcb67386b9c09408284fe4f798be6710eb2..d0f2381e87bcb52460d9d4cb3b9c9651fd9d66c4 100644 (file)
@@ -95,28 +95,27 @@ namespace {
 // Generally, after the execution of a full findBasePointer call, only the
 // base relation will remain.  Internally, we add a mixture of the two
 // types, then update all the second type to the first type
-typedef std::map<Value *, Value *> DefiningValueMapTy;
+typedef DenseMap<Value *, Value *> DefiningValueMapTy;
+typedef DenseSet<llvm::Value *> StatepointLiveSetTy;
 
 struct PartiallyConstructedSafepointRecord {
   /// The set of values known to be live accross this safepoint
-  std::set<llvm::Value *> liveset;
+  StatepointLiveSetTy liveset;
 
   /// Mapping from live pointers to a base-defining-value
-  std::map<llvm::Value *, llvm::Value *> base_pairs;
+  DenseMap<llvm::Value *, llvm::Value *> PointerToBase;
 
   /// Any new values which were added to the IR during base pointer analysis
   /// for this safepoint
-  std::set<llvm::Value *> newInsertedDefs;
+  DenseSet<llvm::Value *> NewInsertedDefs;
 
-  /// The bounds of the inserted code for the safepoint
-  std::pair<Instruction *, Instruction *> safepoint;
+  /// The *new* gc.statepoint instruction itself.  This produces the token
+  /// that normal path gc.relocates and the gc.result are tied to.
+  Instruction *StatepointToken;
 
-  // Instruction to which exceptional gc relocates are attached
-  // Makes it easier to iterate through them during relocationViaAlloca.
-  Instruction *exceptional_relocates_token;
-
-  /// The result of the safepointing call (or nullptr)
-  Value *result;
+  /// Instruction to which exceptional gc relocates are attached
+  /// Makes it easier to iterate through them during relocationViaAlloca.
+  Instruction *UnwindToken;
 };
 }
 
@@ -132,12 +131,54 @@ static bool isGCPointerType(const Type *T) {
   return false;
 }
 
+// Return true if this type is one which a) is a gc pointer or contains a GC
+// pointer and b) is of a type this code expects to encounter as a live value.
+// (The insertion code will assert that a type which matches (a) and not (b)
+// is not encountered.) 
+static bool isHandledGCPointerType(Type *T) {
+  // We fully support gc pointers
+  if (isGCPointerType(T))
+    return true;
+  // We partially support vectors of gc pointers. The code will assert if it
+  // can't handle something.
+  if (auto VT = dyn_cast<VectorType>(T))
+    if (isGCPointerType(VT->getElementType()))
+      return true;
+  return false;
+}
+
+#ifndef NDEBUG
+/// Returns true if this type contains a gc pointer whether we know how to
+/// handle that type or not.
+static bool containsGCPtrType(Type *Ty) {
+  if(isGCPointerType(Ty))
+    return true;
+  if (VectorType *VT = dyn_cast<VectorType>(Ty))
+    return isGCPointerType(VT->getScalarType());
+  if (ArrayType *AT = dyn_cast<ArrayType>(Ty))
+    return containsGCPtrType(AT->getElementType());
+  if (StructType *ST = dyn_cast<StructType>(Ty))
+    return std::any_of(ST->subtypes().begin(), ST->subtypes().end(),
+                       [](Type *SubType) {
+                         return containsGCPtrType(SubType);
+                       });
+  return false;
+}
+
+// Returns true if this is a type which a) is a gc pointer or contains a GC
+// pointer and b) is of a type which the code doesn't expect (i.e. first class
+// aggregates).  Used to trip assertions.
+static bool isUnhandledGCPointerType(Type *Ty) {
+  return containsGCPtrType(Ty) && !isHandledGCPointerType(Ty);
+}
+#endif
+
 /// Return true if the Value is a gc reference type which is potentially used
 /// after the instruction 'loc'.  This is only used with the edge reachability
 /// liveness code.  Note: It is assumed the V dominates loc.
-static bool isLiveGCReferenceAt(Value &V, Instruction *loc, DominatorTree &DT,
+static bool isLiveGCReferenceAt(Value &V, Instruction *Loc, DominatorTree &DT,
                                 LoopInfo *LI) {
-  if (!isGCPointerType(V.getType()))
+  if (!isHandledGCPointerType(V.getType()))
     return false;
 
   if (V.use_empty())
@@ -147,24 +188,6 @@ static bool isLiveGCReferenceAt(Value &V, Instruction *loc, DominatorTree &DT,
   return true;
 }
 
-#ifndef NDEBUG
-static bool isAggWhichContainsGCPtrType(Type *Ty) {
-  if (VectorType *VT = dyn_cast<VectorType>(Ty))
-    return isGCPointerType(VT->getScalarType());
-  else if (ArrayType *AT = dyn_cast<ArrayType>(Ty)) {
-    return isGCPointerType(AT->getElementType()) ||
-           isAggWhichContainsGCPtrType(AT->getElementType());
-  } else if (StructType *ST = dyn_cast<StructType>(Ty)) {
-    bool UnsupportedType = false;
-    for (Type *SubType : ST->subtypes())
-      UnsupportedType |=
-          isGCPointerType(SubType) || isAggWhichContainsGCPtrType(SubType);
-    return UnsupportedType;
-  } else
-    return false;
-}
-#endif
-
 // Conservatively identifies any definitions which might be live at the
 // given instruction. The  analysis is performed immediately before the
 // given instruction. Values defined by that instruction are not considered
@@ -178,7 +201,7 @@ static bool isAggWhichContainsGCPtrType(Type *Ty) {
 //  postconditions: populates liveValues as discussed above
 static void findLiveGCValuesAtInst(Instruction *term, BasicBlock *pred,
                                    DominatorTree &DT, LoopInfo *LI,
-                                   std::set<llvm::Value *> &liveValues) {
+                                   StatepointLiveSetTy &liveValues) {
   liveValues.clear();
 
   assert(isa<CallInst>(term) || isa<InvokeInst>(term) || term->isTerminator());
@@ -191,7 +214,7 @@ static void findLiveGCValuesAtInst(Instruction *term, BasicBlock *pred,
   // Are there any gc pointer arguments live over this point?  This needs to be
   // special cased since arguments aren't defined in basic blocks.
   for (Argument &arg : F->args()) {
-    assert(!isAggWhichContainsGCPtrType(arg.getType()) &&
+    assert(!isUnhandledGCPointerType(arg.getType()) &&
            "support for FCA unimplemented");
 
     if (is_live_gc_reference(arg)) {
@@ -235,7 +258,7 @@ static void findLiveGCValuesAtInst(Instruction *term, BasicBlock *pred,
         break;
       }
 
-      assert(!isAggWhichContainsGCPtrType(inst.getType()) &&
+      assert(!isUnhandledGCPointerType(inst.getType()) &&
              "support for FCA unimplemented");
 
       if (is_live_gc_reference(inst)) {
@@ -278,14 +301,14 @@ analyzeParsePointLiveness(DominatorTree &DT, const CallSite &CS,
   Instruction *inst = CS.getInstruction();
 
   BasicBlock *BB = inst->getParent();
-  std::set<Value *> liveset;
+  StatepointLiveSetTy liveset;
   findLiveGCValuesAtInst(inst, BB, DT, nullptr, liveset);
 
   if (PrintLiveSet) {
     // Note: This output is used by several of the test cases
     // The order of elemtns in a set is not stable, put them in a vec and sort
     // by name
-    std::vector<Value *> temp;
+    SmallVector<Value *, 64> temp;
     temp.insert(temp.end(), liveset.begin(), liveset.end());
     std::sort(temp.begin(), temp.end(), order_by_name);
     errs() << "Live Variables:\n";
@@ -301,10 +324,49 @@ analyzeParsePointLiveness(DominatorTree &DT, const CallSite &CS,
   result.liveset = liveset;
 }
 
-/// True iff this value is the null pointer constant (of any pointer type)
-static bool isNullConstant(Value *V) {
-  return isa<Constant>(V) && isa<PointerType>(V->getType()) &&
-         cast<Constant>(V)->isNullValue();
+/// If we can trivially determine that this vector contains only base pointers,
+/// return the base instruction.  
+static Value *findBaseOfVector(Value *I) {
+  assert(I->getType()->isVectorTy() &&
+         cast<VectorType>(I->getType())->getElementType()->isPointerTy() &&
+         "Illegal to ask for the base pointer of a non-pointer type");
+
+  // Each case parallels findBaseDefiningValue below, see that code for
+  // detailed motivation.
+
+  if (isa<Argument>(I))
+    // An incoming argument to the function is a base pointer
+    return I;
+
+  // We shouldn't see the address of a global as a vector value?
+  assert(!isa<GlobalVariable>(I) &&
+         "unexpected global variable found in base of vector");
+
+  // inlining could possibly introduce phi node that contains
+  // undef if callee has multiple returns
+  if (isa<UndefValue>(I))
+    // utterly meaningless, but useful for dealing with partially optimized
+    // code.
+    return I; 
+
+  // Due to inheritance, this must be _after_ the global variable and undef
+  // checks
+  if (Constant *Con = dyn_cast<Constant>(I)) {
+    assert(!isa<GlobalVariable>(I) && !isa<UndefValue>(I) &&
+           "order of checks wrong!");
+    assert(Con->isNullValue() && "null is the only case which makes sense");
+    return Con;
+  }
+
+  if (isa<LoadInst>(I))
+    return I;
+
+  // Note: This code is currently rather incomplete.  We are essentially only
+  // handling cases where the vector element is trivially a base pointer.  We
+  // need to update the entire base pointer construction algorithm to know how
+  // to track vector elements and potentially scalarize, but the case which
+  // would motivate the work hasn't shown up in real workloads yet.
+  llvm_unreachable("no base found for vector element");
 }
 
 /// Helper function for findBasePointer - Will return a value which either a)
@@ -314,52 +376,36 @@ static Value *findBaseDefiningValue(Value *I) {
   assert(I->getType()->isPointerTy() &&
          "Illegal to ask for the base pointer of a non-pointer type");
 
-  // There are instructions which can never return gc pointer values.  Sanity
-  // check
-  // that this is actually true.
-  assert(!isa<InsertElementInst>(I) && !isa<ExtractElementInst>(I) &&
-         !isa<ShuffleVectorInst>(I) && "Vector types are not gc pointers");
-  assert((!isa<Instruction>(I) || isa<InvokeInst>(I) ||
-          !cast<Instruction>(I)->isTerminator()) &&
-         "With the exception of invoke terminators don't define values");
-  assert(!isa<StoreInst>(I) && !isa<FenceInst>(I) &&
-         "Can't be definitions to start with");
-  assert(!isa<ICmpInst>(I) && !isa<FCmpInst>(I) &&
-         "Comparisons don't give ops");
-  // There's a bunch of instructions which just don't make sense to apply to
-  // a pointer.  The only valid reason for this would be pointer bit
-  // twiddling which we're just not going to support.
-  assert((!isa<Instruction>(I) || !cast<Instruction>(I)->isBinaryOp()) &&
-         "Binary ops on pointer values are meaningless.  Unless your "
-         "bit-twiddling which we don't support");
-
-  if (Argument *Arg = dyn_cast<Argument>(I)) {
+  // This case is a bit of a hack - it only handles extracts from vectors which
+  // trivially contain only base pointers.  See note inside the function for
+  // how to improve this.
+  if (auto *EEI = dyn_cast<ExtractElementInst>(I)) {
+    Value *VectorOperand = EEI->getVectorOperand();
+    Value *VectorBase = findBaseOfVector(VectorOperand);
+    (void)VectorBase;
+    assert(VectorBase && "extract element not known to be a trivial base");
+    return EEI;
+  }
+
+  if (isa<Argument>(I))
     // An incoming argument to the function is a base pointer
     // We should have never reached here if this argument isn't an gc value
-    assert(Arg->getType()->isPointerTy() &&
-           "Base for pointer must be another pointer");
-    return Arg;
-  }
+    return I;
 
-  if (GlobalVariable *global = dyn_cast<GlobalVariable>(I)) {
+  if (isa<GlobalVariable>(I))
     // base case
-    assert(global->getType()->isPointerTy() &&
-           "Base for pointer must be another pointer");
-    return global;
-  }
+    return I;
 
   // inlining could possibly introduce phi node that contains
   // undef if callee has multiple returns
-  if (UndefValue *undef = dyn_cast<UndefValue>(I)) {
-    assert(undef->getType()->isPointerTy() &&
-           "Base for pointer must be another pointer");
-    return undef; // utterly meaningless, but useful for dealing with
-                  // partially optimized code.
-  }
+  if (isa<UndefValue>(I))
+    // utterly meaningless, but useful for dealing with
+    // partially optimized code.
+    return I; 
 
   // Due to inheritance, this must be _after_ the global variable and undef
   // checks
-  if (Constant *con = dyn_cast<Constant>(I)) {
+  if (Constant *Con = dyn_cast<Constant>(I)) {
     assert(!isa<GlobalVariable>(I) && !isa<UndefValue>(I) &&
            "order of checks wrong!");
     // Note: Finding a constant base for something marked for relocation
@@ -370,52 +416,30 @@ static Value *findBaseDefiningValue(Value *I) {
     // off a potentially null value and have proven it null.  We also use
     // null pointers in dead paths of relocation phis (which we might later
     // want to find a base pointer for).
-    assert(con->getType()->isPointerTy() &&
-           "Base for pointer must be another pointer");
-    assert(con->isNullValue() && "null is the only case which makes sense");
-    return con;
+    assert(isa<ConstantPointerNull>(Con) &&
+           "null is the only case which makes sense");
+    return Con;
   }
 
   if (CastInst *CI = dyn_cast<CastInst>(I)) {
-    Value *def = CI->stripPointerCasts();
-    assert(def->getType()->isPointerTy() &&
-           "Base for pointer must be another pointer");
-    if (isa<CastInst>(def)) {
-      // If we find a cast instruction here, it means we've found a cast
-      // which is not simply a pointer cast (i.e. an inttoptr).  We don't
-      // know how to handle int->ptr conversion.
-      llvm_unreachable("Can not find the base pointers for an inttoptr cast");
-    }
-    assert(!isa<CastInst>(def) && "shouldn't find another cast here");
-    return findBaseDefiningValue(def);
+    Value *Def = CI->stripPointerCasts();
+    // If we find a cast instruction here, it means we've found a cast which is
+    // not simply a pointer cast (i.e. an inttoptr).  We don't know how to
+    // handle int->ptr conversion.
+    assert(!isa<CastInst>(Def) && "shouldn't find another cast here");
+    return findBaseDefiningValue(Def);
   }
 
-  if (LoadInst *LI = dyn_cast<LoadInst>(I)) {
-    if (LI->getType()->isPointerTy()) {
-      Value *Op = LI->getOperand(0);
-      (void)Op;
-      // Has to be a pointer to an gc object, or possibly an array of such?
-      assert(Op->getType()->isPointerTy());
-      return LI; // The value loaded is an gc base itself
-    }
-  }
-  if (GetElementPtrInst *GEP = dyn_cast<GetElementPtrInst>(I)) {
-    Value *Op = GEP->getOperand(0);
-    if (Op->getType()->isPointerTy()) {
-      return findBaseDefiningValue(Op); // The base of this GEP is the base
-    }
-  }
+  if (isa<LoadInst>(I))
+    return I; // The value loaded is an gc base itself
 
-  if (AllocaInst *alloc = dyn_cast<AllocaInst>(I)) {
-    // An alloca represents a conceptual stack slot.  It's the slot itself
-    // that the GC needs to know about, not the value in the slot.
-    assert(alloc->getType()->isPointerTy() &&
-           "Base for pointer must be another pointer");
-    return alloc;
-  }
+  if (GetElementPtrInst *GEP = dyn_cast<GetElementPtrInst>(I))
+    // The base of this GEP is the base
+    return findBaseDefiningValue(GEP->getPointerOperand());
 
   if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(I)) {
     switch (II->getIntrinsicID()) {
+    case Intrinsic::experimental_gc_result_ptr:
     default:
       // fall through to general call handling
       break;
@@ -423,11 +447,6 @@ static Value *findBaseDefiningValue(Value *I) {
     case Intrinsic::experimental_gc_result_float:
     case Intrinsic::experimental_gc_result_int:
       llvm_unreachable("these don't produce pointers");
-    case Intrinsic::experimental_gc_result_ptr:
-      // This is just a special case of the CallInst check below to handle a
-      // statepoint with deopt args which hasn't been rewritten for GC yet.
-      // TODO: Assert that the statepoint isn't rewritten yet.
-      return II;
     case Intrinsic::experimental_gc_relocate: {
       // Rerunning safepoint insertion after safepoints are already
       // inserted is not supported.  It could probably be made to work,
@@ -445,41 +464,27 @@ static Value *findBaseDefiningValue(Value *I) {
   // We assume that functions in the source language only return base
   // pointers.  This should probably be generalized via attributes to support
   // both source language and internal functions.
-  if (CallInst *call = dyn_cast<CallInst>(I)) {
-    assert(call->getType()->isPointerTy() &&
-           "Base for pointer must be another pointer");
-    return call;
-  }
-  if (InvokeInst *invoke = dyn_cast<InvokeInst>(I)) {
-    assert(invoke->getType()->isPointerTy() &&
-           "Base for pointer must be another pointer");
-    return invoke;
-  }
+  if (isa<CallInst>(I) || isa<InvokeInst>(I))
+    return I;
 
   // I have absolutely no idea how to implement this part yet.  It's not
   // neccessarily hard, I just haven't really looked at it yet.
   assert(!isa<LandingPadInst>(I) && "Landing Pad is unimplemented");
 
-  if (AtomicCmpXchgInst *cas = dyn_cast<AtomicCmpXchgInst>(I)) {
+  if (isa<AtomicCmpXchgInst>(I))
     // A CAS is effectively a atomic store and load combined under a
     // predicate.  From the perspective of base pointers, we just treat it
-    // like a load.  We loaded a pointer from a address in memory, that value
-    // had better be a valid base pointer.
-    return cas->getPointerOperand();
-  }
-  if (AtomicRMWInst *atomic = dyn_cast<AtomicRMWInst>(I)) {
-    assert(AtomicRMWInst::Xchg == atomic->getOperation() &&
-           "All others are binary ops which don't apply to base pointers");
-    // semantically, a load, store pair.  Treat it the same as a standard load
-    return atomic->getPointerOperand();
-  }
+    // like a load.
+    return I;
+  
+  assert(!isa<AtomicRMWInst>(I) && "Xchg handled above, all others are "
+         "binary ops which don't apply to pointers");
 
   // The aggregate ops.  Aggregates can either be in the heap or on the
   // stack, but in either case, this is simply a field load.  As a result,
   // this is a defining definition of the base just like a load is.
-  if (ExtractValueInst *ev = dyn_cast<ExtractValueInst>(I)) {
-    return ev;
-  }
+  if (isa<ExtractValueInst>(I))
+    return I;
 
   // We should never see an insert vector since that would require we be
   // tracing back a struct value not a pointer value.
@@ -490,28 +495,21 @@ static Value *findBaseDefiningValue(Value *I) {
   // return a value which dynamically selects from amoung several base
   // derived pointers (each with it's own base potentially).  It's the job of
   // the caller to resolve these.
-  if (SelectInst *select = dyn_cast<SelectInst>(I)) {
-    return select;
-  }
-  if (PHINode *phi = dyn_cast<PHINode>(I)) {
-    return phi;
-  }
-
-  errs() << "unknown type: " << *I << "\n";
-  llvm_unreachable("unknown type");
-  return nullptr;
+  assert((isa<SelectInst>(I) || isa<PHINode>(I)) && 
+         "missing instruction case in findBaseDefiningValing");
+  return I;
 }
 
 /// Returns the base defining value for this value.
-static Value *findBaseDefiningValueCached(Value *I, DefiningValueMapTy &cache) {
-  Value *&Cached = cache[I];
+static Value *findBaseDefiningValueCached(Value *I, DefiningValueMapTy &Cache) {
+  Value *&Cached = Cache[I];
   if (!Cached) {
     Cached = findBaseDefiningValue(I);
   }
-  assert(cache[I] != nullptr);
+  assert(Cache[I] != nullptr);
 
   if (TraceLSP) {
-    errs() << "fBDV-cached: " << I->getName() << " -> " << Cached->getName()
+    dbgs() << "fBDV-cached: " << I->getName() << " -> " << Cached->getName()
            << "\n";
   }
   return Cached;
@@ -519,25 +517,26 @@ static Value *findBaseDefiningValueCached(Value *I, DefiningValueMapTy &cache) {
 
 /// Return a base pointer for this value if known.  Otherwise, return it's
 /// base defining value.
-static Value *findBaseOrBDV(Value *I, DefiningValueMapTy &cache) {
-  Value *def = findBaseDefiningValueCached(I, cache);
-  auto Found = cache.find(def);
-  if (Found != cache.end()) {
+static Value *findBaseOrBDV(Value *I, DefiningValueMapTy &Cache) {
+  Value *Def = findBaseDefiningValueCached(I, Cache);
+  auto Found = Cache.find(Def);
+  if (Found != Cache.end()) {
     // Either a base-of relation, or a self reference.  Caller must check.
     return Found->second;
   }
   // Only a BDV available
-  return def;
+  return Def;
 }
 
 /// Given the result of a call to findBaseDefiningValue, or findBaseOrBDV,
 /// is it known to be a base pointer?  Or do we need to continue searching.
-static bool isKnownBaseResult(Value *v) {
-  if (!isa<PHINode>(v) && !isa<SelectInst>(v)) {
+static bool isKnownBaseResult(Value *V) {
+  if (!isa<PHINode>(V) && !isa<SelectInst>(V)) {
     // no recursion possible
     return true;
   }
-  if (cast<Instruction>(v)->getMetadata("is_base_value")) {
+  if (isa<Instruction>(V) &&
+      cast<Instruction>(V)->getMetadata("is_base_value")) {
     // This is a previously inserted base phi or select.  We know
     // that this is a base value.
     return true;
@@ -558,9 +557,6 @@ public:
   }
   PhiState(Value *b) : status(Base), base(b) {}
   PhiState() : status(Unknown), base(nullptr) {}
-  PhiState(const PhiState &other) : status(other.status), base(other.base) {
-    assert(status != Base || base);
-  }
 
   Status getStatus() const { return status; }
   Value *getBase() const { return base; }
@@ -585,13 +581,14 @@ private:
   Value *base; // non null only if status == base
 };
 
+typedef DenseMap<Value *, PhiState> ConflictStateMapTy;
 // Values of type PhiState form a lattice, and this is a helper
 // class that implementes the meet operation.  The meat of the meet
 // operation is implemented in MeetPhiStates::pureMeet
 class MeetPhiStates {
 public:
   // phiStates is a mapping from PHINodes and SelectInst's to PhiStates.
-  explicit MeetPhiStates(const std::map<Value *, PhiState> &phiStates)
+  explicit MeetPhiStates(const ConflictStateMapTy &phiStates)
       : phiStates(phiStates) {}
 
   // Destructively meet the current result with the base V.  V can
@@ -609,7 +606,7 @@ public:
   PhiState getResult() const { return currentResult; }
 
 private:
-  const std::map<Value *, PhiState> &phiStates;
+  const ConflictStateMapTy &phiStates;
   PhiState currentResult;
 
   /// Return a phi state for a base defining value.  We'll generate a new
@@ -635,23 +632,23 @@ private:
 
     case PhiState::Base:
       assert(stateA.getBase() && "can't be null");
-      if (stateB.isUnknown()) {
+      if (stateB.isUnknown())
         return stateA;
-      } else if (stateB.isBase()) {
+
+      if (stateB.isBase()) {
         if (stateA.getBase() == stateB.getBase()) {
           assert(stateA == stateB && "equality broken!");
           return stateA;
         }
         return PhiState(PhiState::Conflict);
-      } else {
-        assert(stateB.isConflict() && "only three states!");
-        return PhiState(PhiState::Conflict);
       }
+      assert(stateB.isConflict() && "only three states!");
+      return PhiState(PhiState::Conflict);
 
     case PhiState::Conflict:
       return stateA;
     }
-    assert(false && "only three states!");
+    llvm_unreachable("only three states!");
   }
 };
 }
@@ -660,7 +657,7 @@ private:
 /// which is the base pointer.  (This is reliable and can be used for
 /// relocation.)  On failure, returns nullptr.
 static Value *findBasePointer(Value *I, DefiningValueMapTy &cache,
-                              std::set<llvm::Value *> &newInsertedDefs) {
+                              DenseSet<llvm::Value *> &NewInsertedDefs) {
   Value *def = findBaseOrBDV(I, cache);
 
   if (isKnownBaseResult(def)) {
@@ -689,22 +686,28 @@ static Value *findBasePointer(Value *I, DefiningValueMapTy &cache,
   // analougous to pessimistic data flow and would likely lead to an
   // overall worse solution.
 
-  std::map<Value *, PhiState> states;
+  ConflictStateMapTy states;
   states[def] = PhiState();
   // Recursively fill in all phis & selects reachable from the initial one
   // for which we don't already know a definite base value for
-  // PERF: Yes, this is as horribly inefficient as it looks.
+  // TODO: This should be rewritten with a worklist
   bool done = false;
   while (!done) {
     done = true;
+    // Since we're adding elements to 'states' as we run, we can't keep
+    // iterators into the set.
+    SmallVector<Value*, 16> Keys;
+    Keys.reserve(states.size());
     for (auto Pair : states) {
-      Value *v = Pair.first;
+      Value *V = Pair.first;
+      Keys.push_back(V);
+    }
+    for (Value *v : Keys) {
       assert(!isKnownBaseResult(v) && "why did it get added?");
       if (PHINode *phi = dyn_cast<PHINode>(v)) {
-        unsigned NumPHIValues = phi->getNumIncomingValues();
-        assert(NumPHIValues > 0 && "zero input phis are illegal");
-        for (unsigned i = 0; i != NumPHIValues; ++i) {
-          Value *InVal = phi->getIncomingValue(i);
+        assert(phi->getNumIncomingValues() > 0 &&
+               "zero input phis are illegal");
+        for (Value *InVal : phi->incoming_values()) {
           Value *local = findBaseOrBDV(InVal, cache);
           if (!isKnownBaseResult(local) && states.find(local) == states.end()) {
             states[local] = PhiState();
@@ -740,26 +743,22 @@ static Value *findBasePointer(Value *I, DefiningValueMapTy &cache,
   // have reached conflict state.  The current version seems too conservative.
 
   bool progress = true;
-  size_t oldSize = 0;
   while (progress) {
-    oldSize = states.size();
+#ifndef NDEBUG
+    size_t oldSize = states.size();
+#endif
     progress = false;
+    // We're only changing keys in this loop, thus safe to keep iterators
     for (auto Pair : states) {
       MeetPhiStates calculateMeet(states);
       Value *v = Pair.first;
       assert(!isKnownBaseResult(v) && "why did it get added?");
-      assert(isa<SelectInst>(v) || isa<PHINode>(v));
       if (SelectInst *select = dyn_cast<SelectInst>(v)) {
         calculateMeet.meetWith(findBaseOrBDV(select->getTrueValue(), cache));
         calculateMeet.meetWith(findBaseOrBDV(select->getFalseValue(), cache));
-      } else if (PHINode *phi = dyn_cast<PHINode>(v)) {
-        for (unsigned i = 0; i < phi->getNumIncomingValues(); i++) {
-          calculateMeet.meetWith(
-              findBaseOrBDV(phi->getIncomingValue(i), cache));
-        }
-      } else {
-        llvm_unreachable("no such state expected");
-      }
+      } else
+        for (Value *Val : cast<PHINode>(v)->incoming_values())
+          calculateMeet.meetWith(findBaseOrBDV(Val, cache));
 
       PhiState oldState = states[v];
       PhiState newState = calculateMeet.getResult();
@@ -784,47 +783,58 @@ static Value *findBasePointer(Value *I, DefiningValueMapTy &cache,
   }
 
   // Insert Phis for all conflicts
+  // We want to keep naming deterministic in the loop that follows, so
+  // sort the keys before iteration.  This is useful in allowing us to
+  // write stable tests. Note that there is no invalidation issue here.
+  SmallVector<Value*, 16> Keys;
+  Keys.reserve(states.size());
   for (auto Pair : states) {
-    Instruction *v = cast<Instruction>(Pair.first);
-    PhiState state = Pair.second;
+    Value *V = Pair.first;
+    Keys.push_back(V);
+  }
+  std::sort(Keys.begin(), Keys.end(), order_by_name);
+  // TODO: adjust naming patterns to avoid this order of iteration dependency
+  for (Value *V : Keys) {
+    Instruction *v = cast<Instruction>(V);
+    PhiState state = states[V];
     assert(!isKnownBaseResult(v) && "why did it get added?");
     assert(!state.isUnknown() && "Optimistic algorithm didn't complete!");
-    if (state.isConflict()) {
-      if (isa<PHINode>(v)) {
-        int num_preds =
-            std::distance(pred_begin(v->getParent()), pred_end(v->getParent()));
-        assert(num_preds > 0 && "how did we reach here");
-        PHINode *phi = PHINode::Create(v->getType(), num_preds, "base_phi", v);
-        newInsertedDefs.insert(phi);
-        // Add metadata marking this as a base value
-        auto *const_1 = ConstantInt::get(
-            Type::getInt32Ty(
-                v->getParent()->getParent()->getParent()->getContext()),
-            1);
-        auto MDConst = ConstantAsMetadata::get(const_1);
-        MDNode *md = MDNode::get(
-            v->getParent()->getParent()->getParent()->getContext(), MDConst);
-        phi->setMetadata("is_base_value", md);
-        states[v] = PhiState(PhiState::Conflict, phi);
-      } else if (SelectInst *sel = dyn_cast<SelectInst>(v)) {
-        // The undef will be replaced later
-        UndefValue *undef = UndefValue::get(sel->getType());
-        SelectInst *basesel = SelectInst::Create(sel->getCondition(), undef,
-                                                 undef, "base_select", sel);
-        newInsertedDefs.insert(basesel);
-        // Add metadata marking this as a base value
-        auto *const_1 = ConstantInt::get(
-            Type::getInt32Ty(
-                v->getParent()->getParent()->getParent()->getContext()),
-            1);
-        auto MDConst = ConstantAsMetadata::get(const_1);
-        MDNode *md = MDNode::get(
-            v->getParent()->getParent()->getParent()->getContext(), MDConst);
-        basesel->setMetadata("is_base_value", md);
-        states[v] = PhiState(PhiState::Conflict, basesel);
-      } else {
-        assert(false);
-      }
+    if (!state.isConflict())
+      continue;
+    
+    if (isa<PHINode>(v)) {
+      int num_preds =
+          std::distance(pred_begin(v->getParent()), pred_end(v->getParent()));
+      assert(num_preds > 0 && "how did we reach here");
+      PHINode *phi = PHINode::Create(v->getType(), num_preds, "base_phi", v);
+      NewInsertedDefs.insert(phi);
+      // Add metadata marking this as a base value
+      auto *const_1 = ConstantInt::get(
+          Type::getInt32Ty(
+              v->getParent()->getParent()->getParent()->getContext()),
+          1);
+      auto MDConst = ConstantAsMetadata::get(const_1);
+      MDNode *md = MDNode::get(
+          v->getParent()->getParent()->getParent()->getContext(), MDConst);
+      phi->setMetadata("is_base_value", md);
+      states[v] = PhiState(PhiState::Conflict, phi);
+    } else {
+      SelectInst *sel = cast<SelectInst>(v);
+      // The undef will be replaced later
+      UndefValue *undef = UndefValue::get(sel->getType());
+      SelectInst *basesel = SelectInst::Create(sel->getCondition(), undef,
+                                               undef, "base_select", sel);
+      NewInsertedDefs.insert(basesel);
+      // Add metadata marking this as a base value
+      auto *const_1 = ConstantInt::get(
+          Type::getInt32Ty(
+              v->getParent()->getParent()->getParent()->getContext()),
+          1);
+      auto MDConst = ConstantAsMetadata::get(const_1);
+      MDNode *md = MDNode::get(
+          v->getParent()->getParent()->getParent()->getContext(), MDConst);
+      basesel->setMetadata("is_base_value", md);
+      states[v] = PhiState(PhiState::Conflict, basesel);
     }
   }
 
@@ -835,97 +845,97 @@ static Value *findBasePointer(Value *I, DefiningValueMapTy &cache,
 
     assert(!isKnownBaseResult(v) && "why did it get added?");
     assert(!state.isUnknown() && "Optimistic algorithm didn't complete!");
-    if (state.isConflict()) {
-      if (PHINode *basephi = dyn_cast<PHINode>(state.getBase())) {
-        PHINode *phi = cast<PHINode>(v);
-        unsigned NumPHIValues = phi->getNumIncomingValues();
-        for (unsigned i = 0; i < NumPHIValues; i++) {
-          Value *InVal = phi->getIncomingValue(i);
-          BasicBlock *InBB = phi->getIncomingBlock(i);
-
-          // If we've already seen InBB, add the same incoming value
-          // we added for it earlier.  The IR verifier requires phi
-          // nodes with multiple entries from the same basic block
-          // to have the same incoming value for each of those
-          // entries.  If we don't do this check here and basephi
-          // has a different type than base, we'll end up adding two
-          // bitcasts (and hence two distinct values) as incoming
-          // values for the same basic block.
-
-          int blockIndex = basephi->getBasicBlockIndex(InBB);
-          if (blockIndex != -1) {
-            Value *oldBase = basephi->getIncomingValue(blockIndex);
-            basephi->addIncoming(oldBase, InBB);
+    if (!state.isConflict())
+      continue;
+    
+    if (PHINode *basephi = dyn_cast<PHINode>(state.getBase())) {
+      PHINode *phi = cast<PHINode>(v);
+      unsigned NumPHIValues = phi->getNumIncomingValues();
+      for (unsigned i = 0; i < NumPHIValues; i++) {
+        Value *InVal = phi->getIncomingValue(i);
+        BasicBlock *InBB = phi->getIncomingBlock(i);
+
+        // If we've already seen InBB, add the same incoming value
+        // we added for it earlier.  The IR verifier requires phi
+        // nodes with multiple entries from the same basic block
+        // to have the same incoming value for each of those
+        // entries.  If we don't do this check here and basephi
+        // has a different type than base, we'll end up adding two
+        // bitcasts (and hence two distinct values) as incoming
+        // values for the same basic block.
+
+        int blockIndex = basephi->getBasicBlockIndex(InBB);
+        if (blockIndex != -1) {
+          Value *oldBase = basephi->getIncomingValue(blockIndex);
+          basephi->addIncoming(oldBase, InBB);
 #ifndef NDEBUG
-            Value *base = findBaseOrBDV(InVal, cache);
-            if (!isKnownBaseResult(base)) {
-              // Either conflict or base.
-              assert(states.count(base));
-              base = states[base].getBase();
-              assert(base != nullptr && "unknown PhiState!");
-              assert(newInsertedDefs.count(base) &&
-                     "should have already added this in a prev. iteration!");
-            }
-
-            // In essense this assert states: the only way two
-            // values incoming from the same basic block may be
-            // different is by being different bitcasts of the same
-            // value.  A cleanup that remains TODO is changing
-            // findBaseOrBDV to return an llvm::Value of the correct
-            // type (and still remain pure).  This will remove the
-            // need to add bitcasts.
-            assert(base->stripPointerCasts() == oldBase->stripPointerCasts() &&
-                   "sanity -- findBaseOrBDV should be pure!");
-#endif
-            continue;
-          }
-
-          // Find either the defining value for the PHI or the normal base for
-          // a non-phi node
           Value *base = findBaseOrBDV(InVal, cache);
           if (!isKnownBaseResult(base)) {
             // Either conflict or base.
             assert(states.count(base));
             base = states[base].getBase();
             assert(base != nullptr && "unknown PhiState!");
+            assert(NewInsertedDefs.count(base) &&
+                   "should have already added this in a prev. iteration!");
           }
-          assert(base && "can't be null");
-          // Must use original input BB since base may not be Instruction
-          // The cast is needed since base traversal may strip away bitcasts
-          if (base->getType() != basephi->getType()) {
-            base = new BitCastInst(base, basephi->getType(), "cast",
-                                   InBB->getTerminator());
-            newInsertedDefs.insert(base);
-          }
-          basephi->addIncoming(base, InBB);
+
+          // In essense this assert states: the only way two
+          // values incoming from the same basic block may be
+          // different is by being different bitcasts of the same
+          // value.  A cleanup that remains TODO is changing
+          // findBaseOrBDV to return an llvm::Value of the correct
+          // type (and still remain pure).  This will remove the
+          // need to add bitcasts.
+          assert(base->stripPointerCasts() == oldBase->stripPointerCasts() &&
+                 "sanity -- findBaseOrBDV should be pure!");
+#endif
+          continue;
         }
-        assert(basephi->getNumIncomingValues() == NumPHIValues);
-      } else if (SelectInst *basesel = dyn_cast<SelectInst>(state.getBase())) {
-        SelectInst *sel = cast<SelectInst>(v);
-        // Operand 1 & 2 are true, false path respectively. TODO: refactor to
-        // something more safe and less hacky.
-        for (int i = 1; i <= 2; i++) {
-          Value *InVal = sel->getOperand(i);
-          // Find either the defining value for the PHI or the normal base for
-          // a non-phi node
-          Value *base = findBaseOrBDV(InVal, cache);
-          if (!isKnownBaseResult(base)) {
-            // Either conflict or base.
-            assert(states.count(base));
-            base = states[base].getBase();
-            assert(base != nullptr && "unknown PhiState!");
-          }
-          assert(base && "can't be null");
-          // Must use original input BB since base may not be Instruction
-          // The cast is needed since base traversal may strip away bitcasts
-          if (base->getType() != basesel->getType()) {
-            base = new BitCastInst(base, basesel->getType(), "cast", basesel);
-            newInsertedDefs.insert(base);
-          }
-          basesel->setOperand(i, base);
+
+        // Find either the defining value for the PHI or the normal base for
+        // a non-phi node
+        Value *base = findBaseOrBDV(InVal, cache);
+        if (!isKnownBaseResult(base)) {
+          // Either conflict or base.
+          assert(states.count(base));
+          base = states[base].getBase();
+          assert(base != nullptr && "unknown PhiState!");
         }
-      } else {
-        assert(false && "unexpected type");
+        assert(base && "can't be null");
+        // Must use original input BB since base may not be Instruction
+        // The cast is needed since base traversal may strip away bitcasts
+        if (base->getType() != basephi->getType()) {
+          base = new BitCastInst(base, basephi->getType(), "cast",
+                                 InBB->getTerminator());
+          NewInsertedDefs.insert(base);
+        }
+        basephi->addIncoming(base, InBB);
+      }
+      assert(basephi->getNumIncomingValues() == NumPHIValues);
+    } else {
+      SelectInst *basesel = cast<SelectInst>(state.getBase());
+      SelectInst *sel = cast<SelectInst>(v);
+      // Operand 1 & 2 are true, false path respectively. TODO: refactor to
+      // something more safe and less hacky.
+      for (int i = 1; i <= 2; i++) {
+        Value *InVal = sel->getOperand(i);
+        // Find either the defining value for the PHI or the normal base for
+        // a non-phi node
+        Value *base = findBaseOrBDV(InVal, cache);
+        if (!isKnownBaseResult(base)) {
+          // Either conflict or base.
+          assert(states.count(base));
+          base = states[base].getBase();
+          assert(base != nullptr && "unknown PhiState!");
+        }
+        assert(base && "can't be null");
+        // Must use original input BB since base may not be Instruction
+        // The cast is needed since base traversal may strip away bitcasts
+        if (base->getType() != basesel->getType()) {
+          base = new BitCastInst(base, basesel->getType(), "cast", basesel);
+          NewInsertedDefs.insert(base);
+        }
+        basesel->setOperand(i, base);
       }
     }
   }
@@ -975,29 +985,35 @@ static Value *findBasePointer(Value *I, DefiningValueMapTy &cache,
 // side effects: may insert PHI nodes into the existing CFG, will preserve
 // CFG, will not remove or mutate any existing nodes
 //
-// post condition: base_pairs contains one (derived, base) pair for every
+// post condition: PointerToBase contains one (derived, base) pair for every
 // pointer in live.  Note that derived can be equal to base if the original
 // pointer was a base pointer.
-static void findBasePointers(const std::set<llvm::Value *> &live,
-                             std::map<llvm::Value *, llvm::Value *> &base_pairs,
+static void findBasePointers(const StatepointLiveSetTy &live,
+                             DenseMap<llvm::Value *, llvm::Value *> &PointerToBase,
                              DominatorTree *DT, DefiningValueMapTy &DVCache,
-                             std::set<llvm::Value *> &newInsertedDefs) {
-  for (Value *ptr : live) {
-    Value *base = findBasePointer(ptr, DVCache, newInsertedDefs);
+                             DenseSet<llvm::Value *> &NewInsertedDefs) {
+  // For the naming of values inserted to be deterministic - which makes for
+  // much cleaner and more stable tests - we need to assign an order to the
+  // live values.  DenseSets do not provide a deterministic order across runs.
+  SmallVector<Value*, 64> Temp;
+  Temp.insert(Temp.end(), live.begin(), live.end());
+  std::sort(Temp.begin(), Temp.end(), order_by_name);
+  for (Value *ptr : Temp) {
+    Value *base = findBasePointer(ptr, DVCache, NewInsertedDefs);
     assert(base && "failed to find base pointer");
-    base_pairs[ptr] = base;
+    PointerToBase[ptr] = base;
     assert((!isa<Instruction>(base) || !isa<Instruction>(ptr) ||
             DT->dominates(cast<Instruction>(base)->getParent(),
                           cast<Instruction>(ptr)->getParent())) &&
            "The base we found better dominate the derived pointer");
 
-    if (isNullConstant(base))
-      // If you see this trip and like to live really dangerously, the code
-      // should be correct, just with idioms the verifier can't handle.  You
-      // can try disabling the verifier at your own substaintial risk.
-      llvm_unreachable("the relocation code needs adjustment to handle the"
-                       "relocation of a null pointer constant without causing"
-                       "false positives in the safepoint ir verifier.");
+    // If you see this trip and like to live really dangerously, the code should
+    // be correct, just with idioms the verifier can't handle.  You can try
+    // disabling the verifier at your own substaintial risk.
+    assert(!isa<ConstantPointerNull>(base) && 
+           "the relocation code needs adjustment to handle the relocation of "
+           "a null pointer constant without causing false positives in the "
+           "safepoint ir verifier.");
   }
 }
 
@@ -1006,31 +1022,40 @@ static void findBasePointers(const std::set<llvm::Value *> &live,
 static void findBasePointers(DominatorTree &DT, DefiningValueMapTy &DVCache,
                              const CallSite &CS,
                              PartiallyConstructedSafepointRecord &result) {
-  std::map<llvm::Value *, llvm::Value *> base_pairs;
-  std::set<llvm::Value *> newInsertedDefs;
-  findBasePointers(result.liveset, base_pairs, &DT, DVCache, newInsertedDefs);
+  DenseMap<llvm::Value *, llvm::Value *> PointerToBase;
+  DenseSet<llvm::Value *> NewInsertedDefs;
+  findBasePointers(result.liveset, PointerToBase, &DT, DVCache, NewInsertedDefs);
 
   if (PrintBasePointers) {
+    // Note: Need to print these in a stable order since this is checked in
+    // some tests.
     errs() << "Base Pairs (w/o Relocation):\n";
-    for (auto Pair : base_pairs) {
-      errs() << " derived %" << Pair.first->getName() << " base %"
-             << Pair.second->getName() << "\n";
+    SmallVector<Value*, 64> Temp;
+    Temp.reserve(PointerToBase.size());
+    for (auto Pair : PointerToBase) {
+      Temp.push_back(Pair.first);
+    }
+    std::sort(Temp.begin(), Temp.end(), order_by_name);
+    for (Value *Ptr : Temp) {
+      Value *Base = PointerToBase[Ptr];
+      errs() << " derived %" << Ptr->getName() << " base %"
+             << Base->getName() << "\n";
     }
   }
 
-  result.base_pairs = base_pairs;
-  result.newInsertedDefs = newInsertedDefs;
+  result.PointerToBase = PointerToBase;
+  result.NewInsertedDefs = NewInsertedDefs;
 }
 
 /// Check for liveness of items in the insert defs and add them to the live
 /// and base pointer sets
 static void fixupLiveness(DominatorTree &DT, const CallSite &CS,
-                          const std::set<Value *> &allInsertedDefs,
+                          const DenseSet<Value *> &allInsertedDefs,
                           PartiallyConstructedSafepointRecord &result) {
   Instruction *inst = CS.getInstruction();
 
-  std::set<llvm::Value *> liveset = result.liveset;
-  std::map<llvm::Value *, llvm::Value *> base_pairs = result.base_pairs;
+  auto liveset = result.liveset;
+  auto PointerToBase = result.PointerToBase;
 
   auto is_live_gc_reference =
       [&](Value &V) { return isLiveGCReferenceAt(V, inst, DT, nullptr); };
@@ -1054,24 +1079,24 @@ static void fixupLiveness(DominatorTree &DT, const CallSite &CS,
     }
 
     if (is_live_gc_reference(*newDef)) {
-      // Add the live new defs into liveset and base_pairs
+      // Add the live new defs into liveset and PointerToBase
       liveset.insert(newDef);
-      base_pairs[newDef] = newDef;
+      PointerToBase[newDef] = newDef;
     }
   }
 
   result.liveset = liveset;
-  result.base_pairs = base_pairs;
+  result.PointerToBase = PointerToBase;
 }
 
 static void fixupLiveReferences(
     Function &F, DominatorTree &DT, Pass *P,
-    const std::set<llvm::Value *> &allInsertedDefs,
-    std::vector<CallSite> &toUpdate,
-    std::vector<struct PartiallyConstructedSafepointRecord> &records) {
+    const DenseSet<llvm::Value *> &allInsertedDefs,
+    ArrayRef<CallSite> toUpdate,
+    MutableArrayRef<struct PartiallyConstructedSafepointRecord> records) {
   for (size_t i = 0; i < records.size(); i++) {
     struct PartiallyConstructedSafepointRecord &info = records[i];
-    CallSite &CS = toUpdate[i];
+    const CallSite &CS = toUpdate[i];
     fixupLiveness(DT, CS, allInsertedDefs, info);
   }
 }
@@ -1099,26 +1124,7 @@ static BasicBlock *normalizeBBForInvokeSafepoint(BasicBlock *BB,
   return ret;
 }
 
-static void
-VerifySafepointBounds(const std::pair<Instruction *, Instruction *> &bounds) {
-  assert(bounds.first->getParent() && bounds.second->getParent() &&
-         "both must belong to basic blocks");
-  if (bounds.first->getParent() == bounds.second->getParent()) {
-    // This is a call safepoint
-    // TODO: scan the range to find the statepoint
-    // TODO: check that the following instruction is not a gc_relocate or
-    // gc_result
-  } else {
-    // This is an invoke safepoint
-    InvokeInst *invoke = dyn_cast<InvokeInst>(bounds.first);
-    (void)invoke;
-    assert(invoke && "only continues over invokes!");
-    assert(invoke->getNormalDest() == bounds.second->getParent() &&
-           "safepoint should continue into normal exit block");
-  }
-}
-
-static int find_index(const SmallVectorImpl<Value *> &livevec, Value *val) {
+static int find_index(ArrayRef<Value *> livevec, Value *val) {
   auto itr = std::find(livevec.begin(), livevec.end(), val);
   assert(livevec.end() != itr);
   size_t index = std::distance(livevec.begin(), itr);
@@ -1168,14 +1174,13 @@ static AttributeSet legalizeCallAttributes(AttributeSet AS) {
 ///   statepointToken - statepoint instruction to which relocates should be
 ///   bound.
 ///   Builder - Llvm IR builder to be used to construct new calls.
-/// Returns array with newly created relocates.
-static std::vector<llvm::Instruction *>
-CreateGCRelocates(const SmallVectorImpl<llvm::Value *> &liveVariables,
-                  const int liveStart,
-                  const SmallVectorImpl<llvm::Value *> &basePtrs,
-                  Instruction *statepointToken, IRBuilder<> Builder) {
-
-  std::vector<llvm::Instruction *> newDefs;
+static void CreateGCRelocates(ArrayRef<llvm::Value *> liveVariables,
+                              const int liveStart,
+                              ArrayRef<llvm::Value *> basePtrs,
+                              Instruction *statepointToken,
+                              IRBuilder<> Builder) {
+  SmallVector<Instruction *, 64> NewDefs;
+  NewDefs.reserve(liveVariables.size());
 
   Module *M = statepointToken->getParent()->getParent()->getParent();
 
@@ -1184,7 +1189,7 @@ CreateGCRelocates(const SmallVectorImpl<llvm::Value *> &liveVariables,
     // combination.  This results is some blow up the function declarations in
     // the IR, but removes the need for argument bitcasts which shrinks the IR
     // greatly and makes it much more readable.
-    std::vector<Type *> types;                    // one per 'any' type
+    SmallVector<Type *, 1> types;                    // one per 'any' type
     types.push_back(liveVariables[i]->getType()); // result type
     Value *gc_relocate_decl = Intrinsic::getDeclaration(
         M, Intrinsic::experimental_gc_relocate, types);
@@ -1206,12 +1211,10 @@ CreateGCRelocates(const SmallVectorImpl<llvm::Value *> &liveVariables,
     // fake call.
     cast<CallInst>(reloc)->setCallingConv(CallingConv::Cold);
 
-    newDefs.push_back(cast<Instruction>(reloc));
+    NewDefs.push_back(cast<Instruction>(reloc));
   }
-  assert(newDefs.size() == liveVariables.size() &&
+  assert(NewDefs.size() == liveVariables.size() &&
          "missing or extra redefinition at safepoint");
-
-  return newDefs;
 }
 
 static void
@@ -1244,7 +1247,7 @@ makeStatepointExplicitImpl(const CallSite &CS, /* to replace */
   IRBuilder<> Builder(insertBefore);
   // Copy all of the arguments from the original statepoint - this includes the
   // target, call args, and deopt args
-  std::vector<llvm::Value *> args;
+  SmallVector<llvm::Value *, 64> args;
   args.insert(args.end(), CS.arg_begin(), CS.arg_end());
   // TODO: Clear the 'needs rewrite' flag
 
@@ -1282,7 +1285,7 @@ makeStatepointExplicitImpl(const CallSite &CS, /* to replace */
     Builder.SetInsertPoint(IP);
     Builder.SetCurrentDebugLocation(IP->getDebugLoc());
 
-  } else if (CS.isInvoke()) {
+  } else {
     InvokeInst *toReplace = cast<InvokeInst>(CS.getInstruction());
 
     // Insert the new invoke into the old block.  We'll remove the old one in a
@@ -1317,7 +1320,7 @@ makeStatepointExplicitImpl(const CallSite &CS, /* to replace */
     Instruction *exceptional_token =
         cast<Instruction>(Builder.CreateExtractValue(
             unwindBlock->getLandingPadInst(), idx, "relocate_token"));
-    result.exceptional_relocates_token = exceptional_token;
+    result.UnwindToken = exceptional_token;
 
     // Just throw away return value. We will use the one we got for normal
     // block.
@@ -1333,8 +1336,6 @@ makeStatepointExplicitImpl(const CallSite &CS, /* to replace */
 
     // gc relocates will be generated later as if it were regular call
     // statepoint
-  } else {
-    llvm_unreachable("unexpect type of CallSite");
   }
   assert(token);
 
@@ -1342,45 +1343,24 @@ makeStatepointExplicitImpl(const CallSite &CS, /* to replace */
   token->takeName(CS.getInstruction());
 
   // The GCResult is already inserted, we just need to find it
-  Instruction *gc_result = nullptr;
-  /* scope */ {
-    Instruction *toReplace = CS.getInstruction();
-    assert((toReplace->hasNUses(0) || toReplace->hasNUses(1)) &&
-           "only valid use before rewrite is gc.result");
-    if (toReplace->hasOneUse()) {
-      Instruction *GCResult = cast<Instruction>(*toReplace->user_begin());
-      assert(isGCResult(GCResult));
-      gc_result = GCResult;
-    }
-  }
+#ifndef NDEBUG
+  Instruction *toReplace = CS.getInstruction();
+  assert((toReplace->hasNUses(0) || toReplace->hasNUses(1)) &&
+         "only valid use before rewrite is gc.result");
+  assert(!toReplace->hasOneUse() ||
+         isGCResult(cast<Instruction>(*toReplace->user_begin())));
+#endif
 
   // Update the gc.result of the original statepoint (if any) to use the newly
   // inserted statepoint.  This is safe to do here since the token can't be
   // considered a live reference.
   CS.getInstruction()->replaceAllUsesWith(token);
 
-  // Second, create a gc.relocate for every live variable
-  std::vector<llvm::Instruction *> newDefs =
-      CreateGCRelocates(liveVariables, live_start, basePtrs, token, Builder);
-
-  // Need to pass through the last part of the safepoint block so that we
-  // don't accidentally update uses in a following gc.relocate which is
-  // still conceptually part of the same safepoint.  Gah.
-  Instruction *last = nullptr;
-  if (!newDefs.empty()) {
-    last = newDefs.back();
-  } else if (gc_result) {
-    last = gc_result;
-  } else {
-    last = token;
-  }
-  assert(last && "can't be null");
-  const auto bounds = std::make_pair(token, last);
+  result.StatepointToken = token;
 
-  // Sanity check our results - this is slightly non-trivial due to invokes
-  VerifySafepointBounds(bounds);
+  // Second, create a gc.relocate for every live variable
+  CreateGCRelocates(liveVariables, live_start, basePtrs, token, Builder);
 
-  result.safepoint = bounds;
 }
 
 namespace {
@@ -1396,7 +1376,7 @@ static void stablize_order(SmallVectorImpl<Value *> &basevec,
                            SmallVectorImpl<Value *> &livevec) {
   assert(basevec.size() == livevec.size());
 
-  std::vector<name_ordering> temp;
+  SmallVector<name_ordering, 64> temp;
   for (size_t i = 0; i < basevec.size(); i++) {
     name_ordering v;
     v.base = basevec[i];
@@ -1418,8 +1398,8 @@ static void stablize_order(SmallVectorImpl<Value *> &basevec,
 static void
 makeStatepointExplicit(DominatorTree &DT, const CallSite &CS, Pass *P,
                        PartiallyConstructedSafepointRecord &result) {
-  std::set<llvm::Value *> liveset = result.liveset;
-  std::map<llvm::Value *, llvm::Value *> base_pairs = result.base_pairs;
+  auto liveset = result.liveset;
+  auto PointerToBase = result.PointerToBase;
 
   // Convert to vector for efficient cross referencing.
   SmallVector<Value *, 64> basevec, livevec;
@@ -1428,8 +1408,8 @@ makeStatepointExplicit(DominatorTree &DT, const CallSite &CS, Pass *P,
   for (Value *L : liveset) {
     livevec.push_back(L);
 
-    assert(base_pairs.find(L) != base_pairs.end());
-    Value *base = base_pairs[L];
+    assert(PointerToBase.find(L) != PointerToBase.end());
+    Value *base = PointerToBase[L];
     basevec.push_back(base);
   }
   assert(livevec.size() == basevec.size());
@@ -1484,17 +1464,16 @@ insertRelocationStores(iterator_range<Value::user_iterator> gcRelocs,
 
 /// do all the relocation update via allocas and mem2reg
 static void relocationViaAlloca(
-    Function &F, DominatorTree &DT, const std::vector<Value *> &live,
-    const std::vector<struct PartiallyConstructedSafepointRecord> &records) {
+    Function &F, DominatorTree &DT, ArrayRef<Value *> live,
+    ArrayRef<struct PartiallyConstructedSafepointRecord> records) {
 #ifndef NDEBUG
-  int initialAllocaNum = 0;
-
-  // record initial number of allocas
-  for (inst_iterator itr = inst_begin(F), end = inst_end(F); itr != end;
-       itr++) {
-    if (isa<AllocaInst>(*itr))
-      initialAllocaNum++;
-  }
+  // record initial number of (static) allocas; we'll check we have the same
+  // number when we get done.
+  int InitialAllocaNum = 0;
+  for (auto I = F.getEntryBlock().begin(), E = F.getEntryBlock().end(); 
+       I != E; I++)
+    if (isa<AllocaInst>(*I))
+      InitialAllocaNum++;
 #endif
 
   // TODO-PERF: change data structures, reserve
@@ -1523,42 +1502,56 @@ static void relocationViaAlloca(
   // otherwise we lose the link between statepoint and old def
   for (size_t i = 0; i < records.size(); i++) {
     const struct PartiallyConstructedSafepointRecord &info = records[i];
-    Value *statepoint = info.safepoint.first;
+    Value *Statepoint = info.StatepointToken;
 
     // This will be used for consistency check
     DenseSet<Value *> visitedLiveValues;
 
     // Insert stores for normal statepoint gc relocates
-    insertRelocationStores(statepoint->users(), allocaMap, visitedLiveValues);
+    insertRelocationStores(Statepoint->users(), allocaMap, visitedLiveValues);
 
     // In case if it was invoke statepoint
     // we will insert stores for exceptional path gc relocates.
-    if (isa<InvokeInst>(statepoint)) {
-      insertRelocationStores(info.exceptional_relocates_token->users(),
+    if (isa<InvokeInst>(Statepoint)) {
+      insertRelocationStores(info.UnwindToken->users(),
                              allocaMap, visitedLiveValues);
     }
 
 #ifndef NDEBUG
-    // For consistency check store null's into allocas for values that are not
-    // relocated
-    // by this statepoint.
+    // As a debuging aid, pretend that an unrelocated pointer becomes null at
+    // the gc.statepoint.  This will turn some subtle GC problems into slightly
+    // easier to debug SEGVs
+    SmallVector<AllocaInst *, 64> ToClobber;
     for (auto Pair : allocaMap) {
-      Value *def = Pair.first;
-      Value *alloca = Pair.second;
+      Value *Def = Pair.first;
+      AllocaInst *Alloca = cast<AllocaInst>(Pair.second);
 
       // This value was relocated
-      if (visitedLiveValues.count(def)) {
+      if (visitedLiveValues.count(Def)) {
         continue;
       }
-      // Result should not be relocated
-      if (def == info.result) {
-        continue;
+      ToClobber.push_back(Alloca);
+    }
+
+    auto InsertClobbersAt = [&](Instruction *IP) {
+      for (auto *AI : ToClobber) {
+        auto AIType = cast<PointerType>(AI->getType());
+        auto PT = cast<PointerType>(AIType->getElementType());
+        Constant *CPN = ConstantPointerNull::get(PT);
+        StoreInst *store = new StoreInst(CPN, AI);
+        store->insertBefore(IP);
       }
+    };
 
-      Constant *CPN =
-          ConstantPointerNull::get(cast<PointerType>(def->getType()));
-      StoreInst *store = new StoreInst(CPN, alloca);
-      store->insertBefore(info.safepoint.second);
+    // Insert the clobbering stores.  These may get intermixed with the
+    // gc.results and gc.relocates, but that's fine.  
+    if (auto II = dyn_cast<InvokeInst>(Statepoint)) {
+      InsertClobbersAt(II->getNormalDest()->getFirstInsertionPt());
+      InsertClobbersAt(II->getUnwindDest()->getFirstInsertionPt());
+    } else {
+      BasicBlock::iterator Next(cast<CallInst>(Statepoint));
+      Next++;
+      InsertClobbersAt(Next);
     }
 #endif
   }
@@ -1608,11 +1601,21 @@ static void relocationViaAlloca(
     // store must be inserted after load, otherwise store will be in alloca's
     // use list and an extra load will be inserted before it
     StoreInst *store = new StoreInst(def, alloca);
-    if (isa<Instruction>(def)) {
-      store->insertAfter(cast<Instruction>(def));
+    if (Instruction *inst = dyn_cast<Instruction>(def)) {
+      if (InvokeInst *invoke = dyn_cast<InvokeInst>(inst)) {
+        // InvokeInst is a TerminatorInst so the store need to be inserted
+        // into its normal destination block.
+        BasicBlock *normalDest = invoke->getNormalDest();
+        store->insertBefore(normalDest->getFirstNonPHI());
+      } else {
+        assert(!inst->isTerminator() &&
+               "The only TerminatorInst that can produce a value is "
+               "InvokeInst which is handled above.");
+         store->insertAfter(inst);
+      }
     } else {
       assert((isa<Argument>(def) || isa<GlobalVariable>(def) ||
-              (isa<Constant>(def) && cast<Constant>(def)->isNullValue())) &&
+              isa<ConstantPointerNull>(def)) &&
              "Must be argument or global");
       store->insertAfter(cast<Instruction>(alloca));
     }
@@ -1626,26 +1629,27 @@ static void relocationViaAlloca(
   }
 
 #ifndef NDEBUG
-  for (inst_iterator itr = inst_begin(F), end = inst_end(F); itr != end;
-       itr++) {
-    if (isa<AllocaInst>(*itr))
-      initialAllocaNum--;
-  }
-  assert(initialAllocaNum == 0 && "We must not introduce any extra allocas");
+  for (auto I = F.getEntryBlock().begin(), E = F.getEntryBlock().end(); 
+       I != E; I++)
+    if (isa<AllocaInst>(*I))
+      InitialAllocaNum--;
+  assert(InitialAllocaNum == 0 && "We must not introduce any extra allocas");
 #endif
 }
 
 /// Implement a unique function which doesn't require we sort the input
 /// vector.  Doing so has the effect of changing the output of a couple of
 /// tests in ways which make them less useful in testing fused safepoints.
-template <typename T> static void unique_unsorted(std::vector<T> &vec) {
-  DenseSet<T> seen;
-  std::vector<T> tmp;
-  vec.reserve(vec.size());
-  std::swap(tmp, vec);
-  for (auto V : tmp) {
-    if (seen.insert(V).second) {
-      vec.push_back(V);
+template <typename T> static void unique_unsorted(SmallVectorImpl<T> &Vec) {
+  DenseSet<T> Seen;
+  SmallVector<T, 128> TempVec;
+  TempVec.reserve(Vec.size());
+  for (auto Element : Vec)
+    TempVec.push_back(Element);
+  Vec.clear();
+  for (auto V : TempVec) {
+    if (Seen.insert(V).second) {
+      Vec.push_back(V);
     }
   }
 }
@@ -1660,7 +1664,7 @@ static Function *getUseHolder(Module &M) {
 /// Insert holders so that each Value is obviously live through the entire
 /// liftetime of the call.
 static void insertUseHolderAfter(CallSite &CS, const ArrayRef<Value *> Values,
-                                 std::vector<CallInst *> &holders) {
+                                 SmallVectorImpl<CallInst *> &holders) {
   Module *M = CS.getInstruction()->getParent()->getParent()->getParent();
   Function *Func = getUseHolder(*M);
   if (CS.isCall()) {
@@ -1679,33 +1683,32 @@ static void insertUseHolderAfter(CallSite &CS, const ArrayRef<Value *> Values,
         Func, Values, "", invoke->getUnwindDest()->getFirstInsertionPt());
     holders.push_back(normal_holder);
     holders.push_back(unwind_holder);
-  } else {
-    assert(false && "Unsupported");
-  }
+  } else
+    llvm_unreachable("unsupported call type");
 }
 
 static void findLiveReferences(
-    Function &F, DominatorTree &DT, Pass *P, std::vector<CallSite> &toUpdate,
-    std::vector<struct PartiallyConstructedSafepointRecord> &records) {
+    Function &F, DominatorTree &DT, Pass *P, ArrayRef<CallSite> toUpdate,
+    MutableArrayRef<struct PartiallyConstructedSafepointRecord> records) {
   for (size_t i = 0; i < records.size(); i++) {
     struct PartiallyConstructedSafepointRecord &info = records[i];
-    CallSite &CS = toUpdate[i];
+    const CallSite &CS = toUpdate[i];
     analyzeParsePointLiveness(DT, CS, info);
   }
 }
 
-static void addBasesAsLiveValues(std::set<Value *> &liveset,
-                                 std::map<Value *, Value *> &base_pairs) {
+static void addBasesAsLiveValues(StatepointLiveSetTy &liveset,
+                                 DenseMap<Value *, Value *> &PointerToBase) {
   // Identify any base pointers which are used in this safepoint, but not
   // themselves relocated.  We need to relocate them so that later inserted
   // safepoints can get the properly relocated base register.
   DenseSet<Value *> missing;
   for (Value *L : liveset) {
-    assert(base_pairs.find(L) != base_pairs.end());
-    Value *base = base_pairs[L];
+    assert(PointerToBase.find(L) != PointerToBase.end());
+    Value *base = PointerToBase[L];
     assert(base);
     if (liveset.find(base) == liveset.end()) {
-      assert(base_pairs.find(base) == base_pairs.end());
+      assert(PointerToBase.find(base) == PointerToBase.end());
       // uniqued by set insert
       missing.insert(base);
     }
@@ -1718,13 +1721,124 @@ static void addBasesAsLiveValues(std::set<Value *> &liveset,
   for (Value *base : missing) {
     assert(base);
     liveset.insert(base);
-    base_pairs[base] = base;
+    PointerToBase[base] = base;
   }
-  assert(liveset.size() == base_pairs.size());
+  assert(liveset.size() == PointerToBase.size());
+}
+
+/// Remove any vector of pointers from the liveset by scalarizing them over the
+/// statepoint instruction.  Adds the scalarized pieces to the liveset.  It
+/// would be preferrable to include the vector in the statepoint itself, but
+/// the lowering code currently does not handle that.  Extending it would be
+/// slightly non-trivial since it requires a format change.  Given how rare
+/// such cases are (for the moment?) scalarizing is an acceptable comprimise.
+static void splitVectorValues(Instruction *StatepointInst,
+                              StatepointLiveSetTy& LiveSet, DominatorTree &DT) {
+  SmallVector<Value *, 16> ToSplit;
+  for (Value *V : LiveSet)
+    if (isa<VectorType>(V->getType()))
+      ToSplit.push_back(V);
+
+  if (ToSplit.empty())
+    return;
+
+  Function &F = *(StatepointInst->getParent()->getParent());
+
+  DenseMap<Value*, AllocaInst*> AllocaMap;
+  // First is normal return, second is exceptional return (invoke only)
+  DenseMap<Value*, std::pair<Value*,Value*>> Replacements;
+  for (Value *V : ToSplit) {
+    LiveSet.erase(V);
+
+    AllocaInst *Alloca = new AllocaInst(V->getType(), "",
+                                        F.getEntryBlock().getFirstNonPHI());
+    AllocaMap[V] = Alloca;
+
+    VectorType *VT = cast<VectorType>(V->getType());
+    IRBuilder<> Builder(StatepointInst);
+    SmallVector<Value*, 16> Elements;
+    for (unsigned i = 0; i < VT->getNumElements(); i++)
+      Elements.push_back(Builder.CreateExtractElement(V, Builder.getInt32(i)));
+    LiveSet.insert(Elements.begin(), Elements.end());
+
+    auto InsertVectorReform = [&](Instruction *IP) {
+      Builder.SetInsertPoint(IP);
+      Builder.SetCurrentDebugLocation(IP->getDebugLoc());
+      Value *ResultVec = UndefValue::get(VT);
+      for (unsigned i = 0; i < VT->getNumElements(); i++)
+        ResultVec = Builder.CreateInsertElement(ResultVec, Elements[i],
+                                                Builder.getInt32(i));
+      return ResultVec;
+    };
+
+    if (isa<CallInst>(StatepointInst)) {
+      BasicBlock::iterator Next(StatepointInst);
+      Next++;
+      Instruction *IP = &*(Next);
+      Replacements[V].first = InsertVectorReform(IP);
+      Replacements[V].second = nullptr;
+    } else {
+      InvokeInst *Invoke = cast<InvokeInst>(StatepointInst);
+      // We've already normalized - check that we don't have shared destination
+      // blocks 
+      BasicBlock *NormalDest = Invoke->getNormalDest();
+      assert(!isa<PHINode>(NormalDest->begin()));
+      BasicBlock *UnwindDest = Invoke->getUnwindDest();
+      assert(!isa<PHINode>(UnwindDest->begin()));
+      // Insert insert element sequences in both successors
+      Instruction *IP = &*(NormalDest->getFirstInsertionPt());
+      Replacements[V].first = InsertVectorReform(IP);
+      IP = &*(UnwindDest->getFirstInsertionPt());
+      Replacements[V].second = InsertVectorReform(IP);
+    }
+  }
+  for (Value *V : ToSplit) {
+    AllocaInst *Alloca = AllocaMap[V];
+
+    // Capture all users before we start mutating use lists
+    SmallVector<Instruction*, 16> Users;
+    for (User *U : V->users())
+      Users.push_back(cast<Instruction>(U));
+
+    for (Instruction *I : Users) {
+      if (auto Phi = dyn_cast<PHINode>(I)) {
+        for (unsigned i = 0; i < Phi->getNumIncomingValues(); i++)
+          if (V == Phi->getIncomingValue(i)) {
+            LoadInst *Load = new LoadInst(Alloca, "",
+                                 Phi->getIncomingBlock(i)->getTerminator());
+            Phi->setIncomingValue(i, Load);
+          }
+      } else {
+        LoadInst *Load = new LoadInst(Alloca, "", I);
+        I->replaceUsesOfWith(V, Load);
+      }
+    }
+
+    // Store the original value and the replacement value into the alloca
+    StoreInst *Store = new StoreInst(V, Alloca);
+    if (auto I = dyn_cast<Instruction>(V))
+      Store->insertAfter(I);
+    else
+      Store->insertAfter(Alloca);
+    
+    // Normal return for invoke, or call return
+    Instruction *Replacement = cast<Instruction>(Replacements[V].first);
+    (new StoreInst(Replacement, Alloca))->insertAfter(Replacement);
+    // Unwind return for invoke only
+    Replacement = cast_or_null<Instruction>(Replacements[V].second);
+    if (Replacement)
+      (new StoreInst(Replacement, Alloca))->insertAfter(Replacement);
+  }
+
+  // apply mem2reg to promote alloca to SSA
+  SmallVector<AllocaInst*, 16> Allocas;
+  for (Value *V : ToSplit)
+    Allocas.push_back(AllocaMap[V]);
+  PromoteMemToReg(Allocas, DT);
 }
 
 static bool insertParsePoints(Function &F, DominatorTree &DT, Pass *P,
-                              std::vector<CallSite> &toUpdate) {
+                              SmallVectorImpl<CallSite> &toUpdate) {
 #ifndef NDEBUG
   // sanity check the input
   std::set<CallSite> uniqued;
@@ -1740,7 +1854,7 @@ static bool insertParsePoints(Function &F, DominatorTree &DT, Pass *P,
 
   // A list of dummy calls added to the IR to keep various values obviously
   // live in the IR.  We'll remove all of these when done.
-  std::vector<CallInst *> holders;
+  SmallVector<CallInst *, 64> holders;
 
   // Insert a dummy call with all of the arguments to the vm_state we'll need
   // for the actual safepoint insertion.  This ensures reference arguments in
@@ -1753,13 +1867,15 @@ static bool insertParsePoints(Function &F, DominatorTree &DT, Pass *P,
     SmallVector<Value *, 64> DeoptValues;
     for (Use &U : StatepointCS.vm_state_args()) {
       Value *Arg = cast<Value>(&U);
-      if (isGCPointerType(Arg->getType()))
+      assert(!isUnhandledGCPointerType(Arg->getType()) &&
+             "support for FCA unimplemented");
+      if (isHandledGCPointerType(Arg->getType()))
         DeoptValues.push_back(Arg);
     }
     insertUseHolderAfter(CS, DeoptValues, holders);
   }
 
-  std::vector<struct PartiallyConstructedSafepointRecord> records;
+  SmallVector<struct PartiallyConstructedSafepointRecord, 64> records;
   records.reserve(toUpdate.size());
   for (size_t i = 0; i < toUpdate.size(); i++) {
     struct PartiallyConstructedSafepointRecord info;
@@ -1771,6 +1887,17 @@ static bool insertParsePoints(Function &F, DominatorTree &DT, Pass *P,
   // site.
   findLiveReferences(F, DT, P, toUpdate, records);
 
+  // Do a limited scalarization of any live at safepoint vector values which
+  // contain pointers.  This enables this pass to run after vectorization at
+  // the cost of some possible performance loss.  TODO: it would be nice to
+  // natively support vectors all the way through the backend so we don't need
+  // to scalarize here.
+  for (size_t i = 0; i < records.size(); i++) {
+    struct PartiallyConstructedSafepointRecord &info = records[i];
+    Instruction *statepoint = toUpdate[i].getInstruction();
+    splitVectorValues(cast<Instruction>(statepoint), info.liveset, DT);
+  }
+
   // B) Find the base pointers for each live pointer
   /* scope for caching */ {
     // Cache the 'defining value' relation used in the computation and
@@ -1794,11 +1921,11 @@ static bool insertParsePoints(Function &F, DominatorTree &DT, Pass *P,
   //   gep a + 1
   //   safepoint 2
   //   br loop
-  std::set<llvm::Value *> allInsertedDefs;
+  DenseSet<llvm::Value *> allInsertedDefs;
   for (size_t i = 0; i < records.size(); i++) {
     struct PartiallyConstructedSafepointRecord &info = records[i];
-    allInsertedDefs.insert(info.newInsertedDefs.begin(),
-                           info.newInsertedDefs.end());
+    allInsertedDefs.insert(info.NewInsertedDefs.begin(),
+                           info.NewInsertedDefs.end());
   }
 
   // We insert some dummy calls after each safepoint to definitely hold live
@@ -1811,7 +1938,7 @@ static bool insertParsePoints(Function &F, DominatorTree &DT, Pass *P,
     CallSite &CS = toUpdate[i];
 
     SmallVector<Value *, 128> Bases;
-    for (auto Pair : info.base_pairs) {
+    for (auto Pair : info.PointerToBase) {
       Bases.push_back(Pair.second);
     }
     insertUseHolderAfter(CS, Bases, holders);
@@ -1825,7 +1952,7 @@ static bool insertParsePoints(Function &F, DominatorTree &DT, Pass *P,
   // given statepoint.
   for (size_t i = 0; i < records.size(); i++) {
     struct PartiallyConstructedSafepointRecord &info = records[i];
-    addBasesAsLiveValues(info.liveset, info.base_pairs);
+    addBasesAsLiveValues(info.liveset, info.PointerToBase);
   }
 
   // If we inserted any new values, we need to adjust our notion of what is
@@ -1837,7 +1964,7 @@ static bool insertParsePoints(Function &F, DominatorTree &DT, Pass *P,
     for (size_t i = 0; i < records.size(); i++) {
       struct PartiallyConstructedSafepointRecord &info = records[i];
       errs() << "Base Pairs: (w/Relocation)\n";
-      for (auto Pair : info.base_pairs) {
+      for (auto Pair : info.PointerToBase) {
         errs() << " derived %" << Pair.first->getName() << " base %"
                << Pair.second->getName() << "\n";
       }
@@ -1870,7 +1997,7 @@ static bool insertParsePoints(Function &F, DominatorTree &DT, Pass *P,
   // nodes have single entry (because of normalizeBBForInvokeSafepoint).
   // Just remove them all here.
   for (size_t i = 0; i < records.size(); i++) {
-    Instruction *I = records[i].safepoint.first;
+    Instruction *I = records[i].StatepointToken;
 
     if (InvokeInst *invoke = dyn_cast<InvokeInst>(I)) {
       FoldSingleEntryPHINodes(invoke->getNormalDest());
@@ -1882,7 +2009,7 @@ static bool insertParsePoints(Function &F, DominatorTree &DT, Pass *P,
   }
 
   // Do all the fixups of the original live variables to their relocated selves
-  std::vector<Value *> live;
+  SmallVector<Value *, 128> live;
   for (size_t i = 0; i < records.size(); i++) {
     struct PartiallyConstructedSafepointRecord &info = records[i];
     // We can't simply save the live set from the original insertion.  One of
@@ -1890,7 +2017,7 @@ static bool insertParsePoints(Function &F, DominatorTree &DT, Pass *P,
     // That Value* no longer exists and we need to use the new gc_result.
     // Thankfully, the liveset is embedded in the statepoint (and updated), so
     // we just grab that.
-    Statepoint statepoint(info.safepoint.first);
+    Statepoint statepoint(info.StatepointToken);
     live.insert(live.end(), statepoint.gc_args_begin(),
                 statepoint.gc_args_end());
   }
@@ -1928,19 +2055,46 @@ bool RewriteStatepointsForGC::runOnFunction(Function &F) {
   if (!shouldRewriteStatepointsIn(F))
     return false;
 
-  // Gather all the statepoints which need rewritten.
-  std::vector<CallSite> ParsePointNeeded;
-  for (inst_iterator itr = inst_begin(F), end = inst_end(F); itr != end;
-       itr++) {
+  DominatorTree &DT = getAnalysis<DominatorTreeWrapperPass>().getDomTree();
+  
+  // Gather all the statepoints which need rewritten.  Be careful to only
+  // consider those in reachable code since we need to ask dominance queries
+  // when rewriting.  We'll delete the unreachable ones in a moment.
+  SmallVector<CallSite, 64> ParsePointNeeded;
+  bool HasUnreachableStatepoint = false;
+  for (Instruction &I : inst_range(F)) {
     // TODO: only the ones with the flag set!
-    if (isStatepoint(*itr))
-      ParsePointNeeded.push_back(CallSite(&*itr));
+    if (isStatepoint(I)) {
+      if (DT.isReachableFromEntry(I.getParent()))
+        ParsePointNeeded.push_back(CallSite(&I));
+      else
+        HasUnreachableStatepoint = true;
+    }
   }
 
+  bool MadeChange = false;
+  
+  // Delete any unreachable statepoints so that we don't have unrewritten
+  // statepoints surviving this pass.  This makes testing easier and the
+  // resulting IR less confusing to human readers.  Rather than be fancy, we
+  // just reuse a utility function which removes the unreachable blocks.
+  if (HasUnreachableStatepoint)
+    MadeChange |= removeUnreachableBlocks(F);
+
   // Return early if no work to do.
   if (ParsePointNeeded.empty())
-    return false;
+    return MadeChange;
+
+  // As a prepass, go ahead and aggressively destroy single entry phi nodes.
+  // These are created by LCSSA.  They have the effect of increasing the size
+  // of liveness sets for no good reason.  It may be harder to do this post
+  // insertion since relocations and base phis can confuse things.
+  for (BasicBlock &BB : F)
+    if (BB.getUniquePredecessor()) {
+      MadeChange = true;
+      FoldSingleEntryPHINodes(&BB);
+    }
 
-  DominatorTree &DT = getAnalysis<DominatorTreeWrapperPass>().getDomTree();
-  return insertParsePoints(F, DT, this, ParsePointNeeded);
+  MadeChange |= insertParsePoints(F, DT, this, ParsePointNeeded);
+  return MadeChange;
 }