Refactor normalization for invokes in placestatepoints pass.

[igor-laevsky]

Summary

This change moves basic block normalization for invokes right before replacement of a call with safepoint in "ReplaceWithStatepoint". Previously it was partly done before replacement of calls with safepoint and partly after call replacement but before RAUW's for gc_relocates, which was confusing.

Since "normalizeForInvokeSafepoint" now looks very much alike it's analog in RewriteSafepointsForGC I think it's reasonable to consider creating utility function in BasicBlockUtils.cpp

[reames]

lib/Transforms/Scalar/PlaceSafepoints.cpp

698

Leaving the assertion here would be good.

952

Please just do this as a preprocessing step. If the statepoint we're about to insert is for an invoke, normalize it before starting the insertion process. Having this functionality inside a function called ReplaceWithStatepoint seems misleading.

In RewriteStatepointsForGC, we have:

// When inserting gc.relocates for invokes, we need to be able to insert at
// the top of the successor blocks.  See the comment on
// normalForInvokeSafepoint on exactly what is needed.  Note that this step
// may restructure the CFG.
for (CallSite CS : toUpdate) {
  if (!CS.isInvoke())
    continue;
  InvokeInst *invoke = cast<InvokeInst>(CS.getInstruction());
  normalizeForInvokeSafepoint(invoke->getNormalDest(), invoke->getParent(),
                              P);
  normalizeForInvokeSafepoint(invoke->getUnwindDest(), invoke->getParent(),
                              P);
}

Doing that same thing before calling ReplaceWithStatepoint would seem reasonable.

[igor-laevsky]

Add assertion and move call to normalizeForInvokeSafepoint out from ReplaceWithStatepoint.

I specifically did not wan't to run normalizeForInvokeSafepoint in a separate loop, to make more clear that ReplaceWithStatepoint is dependent on normalizeForInvokeSafepoint being previously run.

[reames]

With comment corrections, LGTM.

lib/Transforms/Scalar/PlaceSafepoints.cpp

699

Comment is incorrect. gc_result, not gc_relocate.

Also, spelling: malformed.

[igor-laevsky]

Committed revision 236829.

lib/Transforms/Scalar/PlaceSafepoints.cpp

 }
 
 // TODO: These should become properties of the GCStrategy, possibly with
 // command line overrides.
 static bool enableEntrySafepoints(Function &F) { return !NoEntry; }
 static bool enableBackedgeSafepoints(Function &F) { return !NoBackedge; }
 static bool enableCallSafepoints(Function &F) { return !NoCall; }
 
+// Normalize basic block to make it ready to be target of invoke statepoint.
+// Ensure that 'BB' does not have phi nodes. It may require spliting it.
+static BasicBlock *normalizeForInvokeSafepoint(BasicBlock *BB,
+                                               BasicBlock *InvokeParent) {
+  BasicBlock *ret = BB;
+
+  if (!BB->getUniquePredecessor()) {
+    ret = SplitBlockPredecessors(BB, InvokeParent, "");
+  }
+
+  // Now that 'ret' has unique predecessor we can safely remove all phi nodes
+  // from it
+  FoldSingleEntryPHINodes(ret);
+  assert(!isa<PHINode>(ret->begin()));
+
+  return ret;
+}
 
 bool PlaceSafepoints::runOnFunction(Function &F) {
   if (F.isDeclaration() || F.empty()) {
     // This is a declaration, nothing to do.  Must exit early to avoid crash in
     // dom tree calculation
     return false;
   }
 
bool PlaceSafepoints::runOnFunction(Function &F) {
 
   // Now run through and insert the safepoints, but do _NOT_ update or remove
   // any existing uses.  We have references to live variables that need to
   // survive to the last iteration of this loop.
   std::vector<Value *> Results;
   Results.reserve(ParsePointNeeded.size());
   for (size_t i = 0; i < ParsePointNeeded.size(); i++) {
     CallSite &CS = ParsePointNeeded[i];
+
+    // For invoke statepoints we need to remove all phi nodes at the normal
+    // destination block.
+    // Reason for this is that we can place gc_relocate only after last phi node
+    // in basic block. We will get mallformed code after RAUW for the
+    // gc_relocate if one of this phi nodes uses result from the invoke.
+    if (InvokeInst *Invoke = dyn_cast<InvokeInst>(CS.getInstruction())) {
+      normalizeForInvokeSafepoint(Invoke->getNormalDest(),
+                                  Invoke->getParent());
+    }
+
     Value *GCResult = ReplaceWithStatepoint(CS, nullptr);
     Results.push_back(GCResult);
   }
   assert(Results.size() == ParsePointNeeded.size());
 
   // Adjust all users of the old call sites to use the new ones instead
   for (size_t i = 0; i < ParsePointNeeded.size(); i++) {
     CallSite &CS = ParsePointNeeded[i];
     Value *GCResult = Results[i];
     if (GCResult) {
-      // In case if we inserted result in a different basic block than the
-      // original safepoint (this can happen for invokes). We need to be sure
-      // that
-      // original result value was not used in any of the phi nodes at the
-      // beginning of basic block with gc result. Because we know that all such
-      // blocks will have single predecessor we can safely assume that all phi
-      // nodes have single entry (because of normalizeBBForInvokeSafepoint).
-      // Just remove them all here.
-      if (CS.isInvoke()) {
-        FoldSingleEntryPHINodes(cast<Instruction>(GCResult)->getParent(),
-                                nullptr);
-        assert(
-            !isa<PHINode>(cast<Instruction>(GCResult)->getParent()->begin()));
-      }
+      // Can not RAUW for the gc result in case of phi nodes preset.
+      assert(!isa<PHINode>(cast<Instruction>(GCResult)->getParent()->begin()));
 
       // Replace all uses with the new call
       CS.getInstruction()->replaceAllUsesWith(GCResult);
     }
 
     // Now that we've handled all uses, remove the original call itself
     // Note: The insert point can't be the deleted instruction!
     CS.getInstruction()->eraseFromParent();
for (size_t i = 0; i < calls.size(); i++) {
 
     // These are likely runtime calls.  Should we assert that via calling
     // convention or something?
     ParsePointsNeeded.push_back(CallSite(calls[i]));
   }
   assert(ParsePointsNeeded.size() <= calls.size());
 }
 
-// Normalize basic block to make it ready to be target of invoke statepoint.
-// It means spliting it to have single predecessor. Return newly created BB
-// ready to be successor of invoke statepoint.
-static BasicBlock *normalizeBBForInvokeSafepoint(BasicBlock *BB,
-                                                 BasicBlock *InvokeParent) {
-  BasicBlock *ret = BB;
-
-  if (!BB->getUniquePredecessor()) {
-    ret = SplitBlockPredecessors(BB, InvokeParent, "");
-  }
-
-  // Another requirement for such basic blocks is to not have any phi nodes.
-  // Since we just ensured that new BB will have single predecessor,
-  // all phi nodes in it will have one value. Here it would be naturall place
-  // to
-  // remove them all. But we can not do this because we are risking to remove
-  // one of the values stored in liveset of another statepoint. We will do it
-  // later after placing all safepoints.
-
-  return ret;
-}
-
 /// Replaces the given call site (Call or Invoke) with a gc.statepoint
 /// intrinsic with an empty deoptimization arguments list.  This does
 /// NOT do explicit relocation for GC support.
 static Value *ReplaceWithStatepoint(const CallSite &CS, /* to replace */
                                     Pass *P) {
   BasicBlock *BB = CS.getInstruction()->getParent();
   Function *F = BB->getParent();
   Module *M = F->getParent();
if (CS.isCall()) {
     InvokeInst *toReplace = cast<InvokeInst>(CS.getInstruction());
 
     // Insert the new invoke into the old block.  We'll remove the old one in a
     // moment at which point this will become the new terminator for the
     // original block.
     InvokeInst *invoke = InvokeInst::Create(
         gc_statepoint_decl, toReplace->getNormalDest(),
         toReplace->getUnwindDest(), args, "", toReplace->getParent());
     invoke->setCallingConv(toReplace->getCallingConv());
 
     // Currently we will fail on parameter attributes and on certain
     // function attributes.
     AttributeSet new_attrs = toReplace->getAttributes();
     // In case if we can handle this set of sttributes - set up function attrs
     // directly on statepoint and return attrs later for gc_result intrinsic.
     invoke->setAttributes(new_attrs.getFnAttributes());
     return_attributes = new_attrs.getRetAttributes();
 
     token = invoke;
 
     // We'll insert the gc.result into the normal block
-    BasicBlock *normalDest = normalizeBBForInvokeSafepoint(
-        toReplace->getNormalDest(), invoke->getParent());
+    BasicBlock *normalDest = toReplace->getNormalDest();
+    // Can not insert gc.result in case of phi nodes preset.
+    // Should have removed this cases prior to runnning this function
+    assert(!isa<PHINode>(normalDest->begin()));
     Instruction *IP = &*(normalDest->getFirstInsertionPt());
     Builder.SetInsertPoint(IP);
   } else {
     llvm_unreachable("unexpect type of CallSite");
   }
   assert(token);
 
   // Handle the return value of the original call - update all uses to use a