[RS4GC] Replace some asserts by similar code using report_fatal_error().

[mjacob]

Summary

I've replaced asserts which check restrictions on the input IR or for
unimplemented cases. The point is to have these checks also in release builds.

[reames]

In general, I don't like the idea of this patch. Can you give a specific reason why you want this?

[mjacob]

At some point in the future I want to be able to compile the language I'm working on using a stable released version of LLVM, which is usually compiled with assertions disabled. Currently invalid input IR can lead to undefined (or at least unwanted) behaviour if assertions are disabled.

Assertions shouldn't be used for input validation. Instead they should be used for catching programming errors. If you think I've rewritten an assertion that falls into the latter category, we can discuss that, though.

[mjacob]

Note: there are two more comments on the mailing list which didn't end up showing here.

I'm abandoning this revision because the verifier should be extended to check restrictions on the input IR instead of adding report_fatal_error() in the RS4GC pass.

lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

if (isa<ShuffleVectorInst>(I))
     // not.  To be conservatively correct, we treat it as a BDV and will
     // duplicate code as needed to construct a parallel vector of bases.
     // TODO: There a number of local optimizations which could be applied here
     // for particular sufflevector patterns.
     return BaseDefiningValueResult(I, false);
 
   // A PHI or Select is a base defining value.  The outer findBasePointer
   // algorithm is responsible for constructing a base value for this BDV.
-  assert((isa<SelectInst>(I) || isa<PHINode>(I)) &&
-         "unknown vector instruction - no base found for vector element");
+  if (isa<SelectInst>(I) || isa<PHINode>(I))
     return BaseDefiningValueResult(I, false);
+
+  report_fatal_error("unknown vector instruction - no base found for vector element");
 }
 
 /// Helper function for findBasePointer - Will return a value which either a)
 /// defines the base pointer for the input, b) blocks the simple search
 /// (i.e. a PHI or Select of two derived pointers), or c) involves a change
 /// from pointer to vector type or back.
 static BaseDefiningValueResult findBaseDefiningValue(Value *I) {
   assert(I->getType()->isPtrOrPtrVectorTy() &&
if (isa<Constant>(I))
     // be introduced by the inliner or the optimizer, especially on dynamically
     // dead paths.  See e.g. test4 in constants.ll.
     return BaseDefiningValueResult(I, true);
 
   if (CastInst *CI = dyn_cast<CastInst>(I)) {
     Value *Def = CI->stripPointerCasts();
     // If stripping pointer casts changes the address space there is an
     // addrspacecast in between.
-    assert(cast<PointerType>(Def->getType())->getAddressSpace() ==
-               cast<PointerType>(CI->getType())->getAddressSpace() &&
-           "unsupported addrspacecast");
+    if (Def->getType()->getPointerAddressSpace() !=
+        CI->getType()->getPointerAddressSpace())
+      report_fatal_error("unsupported addrspacecast");
     // 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");
+    if (isa<CastInst>(Def))
+      report_fatal_error("shouldn't find another cast here");
     return findBaseDefiningValue(Def);
   }
 
   if (isa<LoadInst>(I))
     // The value loaded is an gc base itself
     return BaseDefiningValueResult(I, true);
   
 
static BaseDefiningValueResult 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 (isa<CallInst>(I) || isa<InvokeInst>(I))
     return BaseDefiningValueResult(I, true);
 
   // I have absolutely no idea how to implement this part yet.  It's not
   // necessarily hard, I just haven't really looked at it yet.
-  assert(!isa<LandingPadInst>(I) && "Landing Pad is unimplemented");
+  if (isa<LandingPadInst>(I))
+    report_fatal_error("Landing Pad is unimplemented");
 
   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.
     return BaseDefiningValueResult(I, true);
 
   assert(!isa<AtomicRMWInst>(I) && "Xchg handled above, all others are "
if (isa<ExtractElementInst>(I))
     // handled after the main base pointer inference algorithm to make writing
     // test cases to exercise that code easier.
     return BaseDefiningValueResult(I, false);
 
   // The last two cases here don't return a base pointer.  Instead, they
   // return a value which dynamically selects from among several base
   // derived pointers (each with it's own base potentially).  It's the job of
   // the caller to resolve these.
-  assert((isa<SelectInst>(I) || isa<PHINode>(I)) &&
-         "missing instruction case in findBaseDefiningValing");
+  if (isa<SelectInst>(I) || isa<PHINode>(I))
     return BaseDefiningValueResult(I, false);
+
+  report_fatal_error("missing instruction case in findBaseDefiningValue");
 }
 
 /// Returns the base defining value for this value.
 static Value *findBaseDefiningValueCached(Value *I, DefiningValueMapTy &Cache) {
   Value *&Cached = Cache[I];
   if (!Cached) {
     Cached = findBaseDefiningValue(I).BDV;
     DEBUG(dbgs() << "fBDV-cached: " << I->getName() << " -> "
for (CallSite CS : ToUpdate) {
     SmallVector<Value *, 64> DeoptValues;
 
     iterator_range<const Use *> DeoptStateRange =
         UseDeoptBundles
             ? iterator_range<const Use *>(GetDeoptBundleOperands(CS))
             : iterator_range<const Use *>(Statepoint(CS).vm_state_args());
 
     for (Value *Arg : DeoptStateRange) {
-      assert(!isUnhandledGCPointerType(Arg->getType()) &&
-             "support for FCA unimplemented");
+      if (isUnhandledGCPointerType(Arg->getType()))
+        report_fatal_error("support for FCA unimplemented");
       if (isHandledGCPointerType(Arg->getType()))
         DeoptValues.push_back(Arg);
     }
 
     insertUseHolderAfter(CS, DeoptValues, Holders);
   }
 
   SmallVector<PartiallyConstructedSafepointRecord, 64> Records(ToUpdate.size());
for (BasicBlock::reverse_iterator ritr = rbegin; ritr != rend; ritr++) {
 
     // Don't consider *uses* in PHI nodes, we handle their contribution to
     // predecessor blocks when we seed the LiveOut sets
     if (isa<PHINode>(I))
       continue;
 
     // USE - Add to the LiveIn set for this instruction
     for (Value *V : I->operands()) {
-      assert(!isUnhandledGCPointerType(V->getType()) &&
-             "support for FCA unimplemented");
+      if (isUnhandledGCPointerType(V->getType()))
+        report_fatal_error("support for FCA unimplemented");
       if (isHandledGCPointerType(V->getType()) && !isa<Constant>(V)) {
         // The choice to exclude all things constant here is slightly subtle.
         // There are two independent reasons:
         // - We assume that things which are constant (from LLVM's definition)
         // do not move at runtime.  For example, the address of a global
         // variable is fixed, even though it's contents may not be.
         // - Second, we can't disallow arbitrary inttoptr constants even
         // if the language frontend does.  Optimization passes are free to
         // locally exploit facts without respect to global reachability.  This
         // can create sections of code which are dynamically unreachable and
         // contain just about anything.  (see constants.ll in tests)
         LiveTmp.insert(V);
       }
     }
   }
 }
 
 static void computeLiveOutSeed(BasicBlock *BB, DenseSet<Value *> &LiveTmp) {
 
   for (BasicBlock *Succ : successors(BB)) {
     const BasicBlock::iterator E(Succ->getFirstNonPHI());
     for (BasicBlock::iterator I = Succ->begin(); I != E; I++) {
       PHINode *Phi = cast<PHINode>(&*I);
       Value *V = Phi->getIncomingValueForBlock(BB);
-      assert(!isUnhandledGCPointerType(V->getType()) &&
-             "support for FCA unimplemented");
+      if (isUnhandledGCPointerType(V->getType()))
+        report_fatal_error("support for FCA unimplemented");
       if (isHandledGCPointerType(V->getType()) && !isa<Constant>(V)) {
         LiveTmp.insert(V);
       }
     }
   }
 }
 
 static DenseSet<Value *> computeKillSet(BasicBlock *BB) {