[PATCH] Refactor normalization for invokes in placestatepoints pass.

[Igor Laevsky]

Hi reames,

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

REPOSITORY
  rL LLVM

http://reviews.llvm.org/D9497

Files:
  lib/Transforms/Scalar/PlaceSafepoints.cpp

Index: lib/Transforms/Scalar/PlaceSafepoints.cpp
===================================================================
--- lib/Transforms/Scalar/PlaceSafepoints.cpp
+++ lib/Transforms/Scalar/PlaceSafepoints.cpp
@@ -684,21 +684,6 @@
     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()));
-      }
-
       // Replace all uses with the new call
       CS.getInstruction()->replaceAllUsesWith(GCResult);
     }
@@ -843,23 +828,19 @@
 }
 
 // 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) {
+// 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, "");
   }
 
-  // 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.
+  // Now that 'ret' has unique predecessor we can safely remove all phi nodes
+  // from it
+  FoldSingleEntryPHINodes(ret);
+  assert(!isa<PHINode>(ret->begin()));
 
   return ret;
 }
@@ -949,6 +930,16 @@
 
     InvokeInst *toReplace = cast<InvokeInst>(CS.getInstruction());
 
+    // We can place gc_relocate only after last phi node in normal destination
+    // basic block. We will get mallformed code after RAUW for the gc_relocate
+    // if one of this phi nodes uses result from the invoke.
+    // To bypass this we avoid all phi nodes at the beginning of
+    // normal destination basic block.
+    if (isa<PHINode>(toReplace->getNormalDest()->begin())) {
+      normalizeForInvokeSafepoint(toReplace->getNormalDest(),
+                                  toReplace->getParent());
+    }
+
     // 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.
@@ -968,8 +959,8 @@
     token = invoke;
 
     // We'll insert the gc.result into the normal block
-    BasicBlock *normalDest = normalizeBBForInvokeSafepoint(
-        toReplace->getNormalDest(), invoke->getParent());
+    BasicBlock *normalDest = toReplace->getNormalDest();
+    assert(!isa<PHINode>(normalDest->begin()));
     Instruction *IP = &*(normalDest->getFirstInsertionPt());
     Builder.SetInsertPoint(IP);
   } else {

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D9497.24945.patch
Type: text/x-patch
Size: 3780 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150505/d322b109/attachment.bin>