[RewriteStatepoints] Fix stale parse points

Summary:
RewriteStatepointsForGC collects parse points for further processing.
During the collection if a callsite is found in an unreachable block
(DominatorTree::isReachableFromEntry()) then all unreachable blocks are
removed by removeUnreachableBlocks(). Some of the removed blocks could
have been reachable according to DominatorTree::isReachableFromEntry().
In this case the collected parse points became stale and resulted in a
crash when accessed.

The fix is to unconditionally canonicalize the IR to
removeUnreachableBlocks and then collect the parse points.

The added test crashes with the old version and passes with this patch.

Patch by Yevgeny Rouban!

Reviewed by: Anna

Differential Revision: https://reviews.llvm.org/D43929

llvm-svn: 326748

diff --git a/llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp b/llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
index 99073ae..43740ae 100644 (file)
--- a/llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
+++ b/llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
@@ -2529,30 +2529,31 @@ bool RewriteStatepointsForGC::runOnFunction(Function &F, DominatorTree &DT,
     return 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.
+  DeferredDominance DD(DT);
+  bool MadeChange = removeUnreachableBlocks(F, nullptr, &DD);
+  DD.flush();
+
   // 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 : instructions(F)) {
     // TODO: only the ones with the flag set!
     if (NeedsRewrite(I)) {
-      if (DT.isReachableFromEntry(I.getParent()))
-        ParsePointNeeded.push_back(CallSite(&I));
-      else
-        HasUnreachableStatepoint = true;
+      // NOTE removeUnreachableBlocks() is stronger than
+      // DominatorTree::isReachableFromEntry(). In other words
+      // removeUnreachableBlocks can remove some blocks for which
+      // isReachableFromEntry() returns true.
+      assert(DT.isReachableFromEntry(I.getParent()) &&
+            "no unreachable blocks expected");
+      ParsePointNeeded.push_back(CallSite(&I));
     }
   }
 
-  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 MadeChange;

diff --git a/llvm/test/Transforms/RewriteStatepointsForGC/relocation.ll b/llvm/test/Transforms/RewriteStatepointsForGC/relocation.ll
index daf4a79..8046cb5 100644 (file)
--- a/llvm/test/Transforms/RewriteStatepointsForGC/relocation.ll
+++ b/llvm/test/Transforms/RewriteStatepointsForGC/relocation.ll
@@ -103,6 +103,10 @@ join:                                             ; preds = %else_branch, %if_br
   ret void
 }
 
+declare void @goo(i64)
+
+declare i32 @moo(i64 addrspace(1)*)
+
 ; Make sure a use in a statepoint gets properly relocated at a previous one.  
 ; This is basically just making sure that statepoints aren't accidentally 
 ; treated specially.
@@ -113,11 +117,13 @@ define void @test3(i64 addrspace(1)* %obj) gc "statepoint-example" {
 ; CHECK-NEXT: bitcast
 ; CHECK-NEXT: gc.statepoint
 entry:
-  call void undef(i64 undef) [ "deopt"(i32 0, i32 -1, i32 0, i32 0, i32 0) ]
-  %0 = call i32 undef(i64 addrspace(1)* %obj) [ "deopt"(i32 0, i32 -1, i32 0, i32 0, i32 0) ]
+  call void @goo(i64 undef) [ "deopt"(i32 0, i32 -1, i32 0, i32 0, i32 0) ]
+  %0 = call i32 @moo(i64 addrspace(1)* %obj) [ "deopt"(i32 0, i32 -1, i32 0, i32 0, i32 0) ]
   ret void
 }
 
+declare i8 addrspace(1)* @boo()
+
 ; Check specifically for the case where the result of a statepoint needs to 
 ; be relocated itself
 define void @test4() gc "statepoint-example" {
@@ -127,17 +133,17 @@ define void @test4() gc "statepoint-example" {
 ; CHECK: gc.statepoint
 ; CHECK: [[RELOCATED:%[^ ]+]] = call {{.*}}gc.relocate
 ; CHECK: @use(i8 addrspace(1)* [[RELOCATED]])
-  %1 = call i8 addrspace(1)* undef() [ "deopt"() ]
-  %2 = call i8 addrspace(1)* undef() [ "deopt"() ]
+  %1 = call i8 addrspace(1)* @boo() [ "deopt"() ]
+  %2 = call i8 addrspace(1)* @boo() [ "deopt"() ]
   call void (...) @use(i8 addrspace(1)* %1)
-  unreachable
+  ret void
 }
 
 ; Test updating a phi where not all inputs are live to begin with
 define void @test5(i8 addrspace(1)* %arg) gc "statepoint-example" {
 ; CHECK-LABEL: test5
 entry:
-  %0 = call i8 addrspace(1)* undef() [ "deopt"() ]
+  %0 = call i8 addrspace(1)* @boo() [ "deopt"() ]
   switch i32 undef, label %kill [
     i32 10, label %merge
     i32 13, label %merge
@@ -154,7 +160,7 @@ merge:                                            ; preds = %kill, %entry, %entr
 ; CHECK-DAG: [ %arg.relocated, %entry ]
   %test = phi i8 addrspace(1)* [ null, %kill ], [ %arg, %entry ], [ %arg, %entry ]
   call void (...) @use(i8 addrspace(1)* %test)
-  unreachable
+  ret void
 }
 
 ; Check to make sure we handle values live over an entry statepoint

diff --git a/llvm/test/Transforms/RewriteStatepointsForGC/unreachable-regression.ll b/llvm/test/Transforms/RewriteStatepointsForGC/unreachable-regression.ll
new file mode 100644 (file)
index 0000000..1f781a4
--- /dev/null
+++ b/llvm/test/Transforms/RewriteStatepointsForGC/unreachable-regression.ll
@@ -0,0 +1,34 @@
+; RUN: opt -S -rewrite-statepoints-for-gc < %s | FileCheck %s
+; RUN: opt -S -passes=rewrite-statepoints-for-gc < %s | FileCheck %s
+;
+; Regression test:
+;   After the rewritable callsite collection if any callsite was found
+;   in a block that was reported unreachable by DominanceTree then
+;   removeUnreachableBlocks() was called. But it is stronger than
+;   DominatorTree::isReachableFromEntry(), i.e. removeUnreachableBlocks
+;   can remove some blocks for which isReachableFromEntry() returns true.
+;   This resulted in stale pointers to the collected but removed
+;   callsites. Such stale pointers caused crash when accessed.
+declare void @f(i8 addrspace(1)* %obj)
+
+define void @test(i8 addrspace(1)* %arg) gc "statepoint-example" {
+; CHECK-LABEL: test(
+; CHECK-NEXT: @f
+ call void @f(i8 addrspace(1)* %arg) #1
+ br i1 true, label %not_zero, label %zero
+
+not_zero:
+ ret void
+
+; This block is reachable but removed by removeUnreachableBlocks()
+zero:
+; CHECK-NOT: @f
+ call void @f(i8 addrspace(1)* %arg) #1
+ ret void
+
+unreach:
+ call void @f(i8 addrspace(1)* %arg) #1
+ ret void
+}
+
+attributes #1 = { norecurse noimplicitfloat }

diff --git a/llvm/test/Transforms/RewriteStatepointsForGC/vector-bitcast.ll b/llvm/test/Transforms/RewriteStatepointsForGC/vector-bitcast.ll
index 74943fc..440042d 100644 (file)
--- a/llvm/test/Transforms/RewriteStatepointsForGC/vector-bitcast.ll
+++ b/llvm/test/Transforms/RewriteStatepointsForGC/vector-bitcast.ll
@@ -7,8 +7,10 @@
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128-ni:1"
 target triple = "x86_64-unknown-linux-gnu"
 
+declare i8 addrspace(1)* @foo()
+
 ; Function Attrs: uwtable
-define void @test() gc "statepoint-example" {
+define i32 @test() gc "statepoint-example" {
 ; CHECK-LABEL: @test
 entry:
 ; CHECK-LABEL: entry
@@ -21,7 +23,7 @@ entry:
 ; CHECK: load atomic
   %bc = bitcast <8 x i8 addrspace(1)*> undef to <8 x i32 addrspace(1)*>
   %ptr= extractelement <8 x i32 addrspace(1)*> %bc, i32 7
-  %0 = call i8 addrspace(1)* undef() [ "deopt"() ]
+  %0 = call i8 addrspace(1)* @foo() [ "deopt"() ]
   %1 = load atomic i32, i32 addrspace(1)* %ptr unordered, align 4
-  unreachable
+  ret i32 %1
 }