commit 89c5491a72d2eda952f8314e6700e8657e13ffb6
author Sanjoy Das <sanjoy@playingwithpointers.com> Mon May 11 18:49:34 2015 +0000
committer Sanjoy Das <sanjoy@playingwithpointers.com> Mon May 11 18:49:34 2015 +0000
tree 1ce36270d68aca3464a611392fb1247731002b0b
parent 539175428883f84c4c8c8e6cc051a5d5b02fde88

[RewriteStatepointsForGC] Fix a bug on creating gc_relocate for pointer to vector of pointers

Summary:
In RewriteStatepointsForGC pass, we create a gc_relocate intrinsic for
each relocated pointer, and the gc_relocate has the same type with the
pointer. During the creation of gc_relocate intrinsic, llvm requires to
mangle its type. However, llvm does not support mangling of all possible
types. RewriteStatepointsForGC will hit an assertion failure when it
tries to create a gc_relocate for pointer to vector of pointers because
mangling for vector of pointers is not supported.

This patch changes the way RewriteStatepointsForGC pass creates
gc_relocate. For each relocated pointer, we erase the type of pointers
and create an unified gc_relocate of type i8 addrspace(1)*. Then a
bitcast is inserted to convert the gc_relocate to the correct type. In
this way, gc_relocate does not need to deal with different types of
pointers and the unsupported type mangling is no longer a problem. This
change would also ease further merge when LLVM erases types of pointers
and introduces an unified pointer type.

Some minor changes are also introduced to gc_relocate related part in
InstCombineCalls, CodeGenPrepare, and Verifier accordingly.

Patch by Chen Li!

Reviewers: reames, AndyAyers, sanjoy

Reviewed By: sanjoy

Subscribers: llvm-commits

Differential Revision: http://reviews.llvm.org/D9592

llvm-svn: 237009

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
index cae611f..3b925ed 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
@@ -1202,6 +1202,7 @@
     // preserve relocation semantics.
     GCRelocateOperands Operands(II);
     Value *DerivedPtr = Operands.getDerivedPtr();
+    auto *GCRelocateType = cast<PointerType>(II->getType());
 
     // Remove the relocation if unused, note that this check is required
     // to prevent the cases below from looping forever.
@@ -1212,14 +1213,18 @@
     // TODO: provide a hook for this in GCStrategy.  This is clearly legal for
     // most practical collectors, but there was discussion in the review thread
     // about whether it was legal for all possible collectors.
-    if (isa<UndefValue>(DerivedPtr))
-      return ReplaceInstUsesWith(*II, DerivedPtr);
+    if (isa<UndefValue>(DerivedPtr)) {
+      // gc_relocate is uncasted. Use undef of gc_relocate's type to replace it.
+      return ReplaceInstUsesWith(*II, UndefValue::get(GCRelocateType));
+    }
 
     // The relocation of null will be null for most any collector.
     // TODO: provide a hook for this in GCStrategy.  There might be some weird
     // collector this property does not hold for.
-    if (isa<ConstantPointerNull>(DerivedPtr))
-      return ReplaceInstUsesWith(*II, DerivedPtr);
+    if (isa<ConstantPointerNull>(DerivedPtr)) {
+      // gc_relocate is uncasted. Use null-pointer of gc_relocate's type to replace it.
+      return ReplaceInstUsesWith(*II, ConstantPointerNull::get(GCRelocateType));
+    }
 
     // isKnownNonNull -> nonnull attribute
     if (isKnownNonNull(DerivedPtr))

llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp

diff --git a/llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp b/llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
index 9cb0748..7f33295 100644
--- a/llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
+++ b/llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
@@ -1075,7 +1075,10 @@
     // the IR, but removes the need for argument bitcasts which shrinks the IR
     // greatly and makes it much more readable.
     SmallVector<Type *, 1> types;                 // one per 'any' type
-    types.push_back(liveVariables[i]->getType()); // result type
+    // All gc_relocate are set to i8 addrspace(1)* type. This could help avoid
+    // cases where the actual value's type mangling is not supported by llvm. A
+    // bitcast is added later to convert gc_relocate to the actual value's type.
+    types.push_back(Type::getInt8PtrTy(M->getContext(), 1));
     Value *gc_relocate_decl = Intrinsic::getDeclaration(
         M, Intrinsic::experimental_gc_relocate, types);
 
@@ -1342,8 +1345,16 @@
     Value *alloca = allocaMap[originalValue];
 
     // Emit store into the related alloca
-    StoreInst *store = new StoreInst(relocatedValue, alloca);
-    store->insertAfter(relocatedValue);
+    // All gc_relocate are i8 addrspace(1)* typed, and it must be bitcasted to
+    // the correct type according to alloca.
+    assert(relocatedValue->getNextNode() && "Should always have one since it's not a terminator");
+    IRBuilder<> Builder(relocatedValue->getNextNode());
+    Value *CastedRelocatedValue =
+        Builder.CreateBitCast(relocatedValue, cast<AllocaInst>(alloca)->getAllocatedType(),
+        relocatedValue->hasName() ? relocatedValue->getName() + ".casted" : "");
+
+    StoreInst *store = new StoreInst(CastedRelocatedValue, alloca);
+    store->insertAfter(cast<Instruction>(CastedRelocatedValue));
 
 #ifndef NDEBUG
     visitedLiveValues.insert(originalValue);