[PATCH] D39388: [RS4GC] Strip off invariant.start because memory locations arent invariant

[Anna Thomas via Phabricator via llvm-commits]

anna created this revision.

Invariant.start on memory locations has the property that the memory
location is unchanging. However, this is not true in the face of
rewriting statepoints for GC.
Teach RS4GC about removing invariant.start so that optimizations after
RS4GC does not incorrect sink a load from the memory location past a
statepoint.

Added test showcasing the issue.


https://reviews.llvm.org/D39388

Files:
  lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
  test/Transforms/RewriteStatepointsForGC/drop-invalid-metadata.ll


Index: test/Transforms/RewriteStatepointsForGC/drop-invalid-metadata.ll
===================================================================
--- test/Transforms/RewriteStatepointsForGC/drop-invalid-metadata.ll
+++ test/Transforms/RewriteStatepointsForGC/drop-invalid-metadata.ll
@@ -75,6 +75,35 @@
   ret void
 }
 
+; invariant.start allows us to sink the load past the baz statepoint call into taken block, which is
+; incorrect. remove the invariant.start and RAUW undef.
+define void @test_inv_start(i1 %cond, i32 addrspace(1)* addrspace(1)* %p, i32 %x, i32 addrspace(1)* %q) gc "statepoint-example" {
+; CHECK-LABEL: test_inv_start
+; CHECK-NOT: invariant.start
+; CHECK: gc.statepoint
+  %v1 = load i32 addrspace(1)*, i32 addrspace(1)* addrspace(1)* %p
+  %invst = call {}* @llvm.invariant.start.p1i32(i64 1, i32 addrspace(1)* %v1)
+  %v2 = load i32, i32 addrspace(1)* %v1
+  call void @baz(i32 %x)
+  br i1 %cond, label %taken, label %untaken
+
+; CHECK-LABEL: taken:
+; CHECK: call void @llvm.invariant.end.p1i32({}* undef, i64 4, i32 addrspace(1)* %v1.relocated.casted)
+taken:
+  store i32 %v2, i32 addrspace(1)* %q, align 16
+  call void @llvm.invariant.end.p1i32({}* %invst, i64 4, i32 addrspace(1)* %v1)
+  ret void
+
+; CHECK-LABEL: untaken:
+; CHECK: gc.statepoint
+untaken:
+  call void @escaping.invariant.start({}* %invst)
+  ret void
+}
+
+declare {}* @llvm.invariant.start.p1i32(i64, i32 addrspace(1)*  nocapture) nounwind readonly
+declare void @llvm.invariant.end.p1i32({}*, i64, i32 addrspace(1)* nocapture) nounwind
+declare void @escaping.invariant.start({}*) nounwind
 declare token @llvm.experimental.gc.statepoint.p0f_isVoidi32f(i64, i32, void (i32)*, i32, i32, ...)
 
 ; Function Attrs: nounwind readonly
Index: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
===================================================================
--- lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
+++ lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
@@ -2392,7 +2392,23 @@
   LLVMContext &Ctx = F.getContext();
   MDBuilder Builder(Ctx);
 
+  // Set of invariantstart instructions that we need to remove.
+  // Use this to avoid invalidating the instruction iterator.
+  SmallVector<IntrinsicInst*, 12> InvariantStartInstructions;
+
   for (Instruction &I : instructions(F)) {
+    // invariant.start on memory location implies that the referenced memory
+    // location is constant and unchanging. This is no longer true after
+    // RewriteStatepointsForGC runs because there can be calls to gc.statepoint
+    // which frees the entire heap and the presence of invariant.start allows
+    // the optimizer to sink the load of a memory location past a statepoint,
+    // which is incorrect.
+    if (auto *II = dyn_cast<IntrinsicInst>(&I))
+      if (II->getIntrinsicID() == Intrinsic::invariant_start) {
+        InvariantStartInstructions.push_back(II);
+        continue;
+      }
+
     if (const MDNode *MD = I.getMetadata(LLVMContext::MD_tbaa)) {
       assert(MD->getNumOperands() < 5 && "unrecognized metadata shape!");
       bool IsImmutableTBAA =
@@ -2422,6 +2438,14 @@
         RemoveNonValidAttrAtIndex(Ctx, CS, AttributeList::ReturnIndex);
     }
   }
+
+  // Delete the invariant.start instructions.
+  for (auto *II : InvariantStartInstructions) {
+    // We cannot just delete the uses of invariant.start (it could be used
+    // as an argument to another call), so we RAUW undef.
+    II->replaceAllUsesWith(UndefValue::get(II->getType()));
+    II->eraseFromParent();
+  }
 }
 
 /// Returns true if this function should be rewritten by this pass.  The main


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D39388.120691.patch
Type: text/x-patch
Size: 3597 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171027/62fa9088/attachment.bin>