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

[Chen Li]

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:627
@@ +626,3 @@
+    if (RelocatedBase->getType() != Base->getType()) {
+      ActualRelocatedBase =
+          cast<Instruction>(Builder.CreateBitCast(RelocatedBase, Base->getType()));
----------------
sanjoy wrote:
> Why not directly insert `ActualRelocatedBase` after `RelocatedBase` by something like:
> 
> ```
> if (...) {
>   IRBuilder::InsertPointGuard IPG(RelocatedBase->getNextNode());
>   Builder.CreateBitCast ...
> ```
I basically try to follow the existing code:


```
Value *Replacement = Builder.CreateGEP(
       Derived->getSourceElementType(), RelocatedBase, makeArrayRef(OffsetV));
Instruction *ReplacementInst = cast<Instruction>(Replacement);
ReplacementInst->removeFromParent();
ReplacementInst->insertAfter(RelocatedBase);
```

I wouldn't mind to use either of the styles, but I would prefer to do a separate patch if we need to change.

================
Comment at: lib/IR/Verifier.cpp:3365
@@ -3368,1 +3364,3 @@
+    // gc_relocate does not need to be the same type as the relocated pointer.
+    // It can casted to the correct type later if it's desired
     break;
----------------
sanjoy wrote:
> As a separate step, do you think it makes sense to erase the types of the pointers being relocated as well (when they're being fed into the `statepoint` call)?  We'll have to do something like that anyway once LLVM has a unified `ptr` pointer type.
I think we should do it if it follows LLVM's direction.

================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:1212
@@ +1211,3 @@
+      // gc_relocate is uncasted. Use undef of gc_relocate's type to replace it.
+      return ReplaceInstUsesWith(*II, UndefValue::get(II->getType()));
+    }
----------------
sanjoy wrote:
> Maybe have a `auto *GCRelocateType = cast<PointerType>(II->getType());` hoisted out?
Will do

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1080
@@ +1079,3 @@
+    // 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));
----------------
sanjoy wrote:
> As a separate change, does it make sense to get rid of gc relocates being type-polymorphic on its return type and just have one `gc_relocate` intrinsic.
That would make sense.

================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:1357
@@ +1356,3 @@
+    Instruction *valueToStore = nullptr;
+    // gc_relocate are just created and haven't been relocated yet. If it has
+    // no use, it means the actual relocated value's type is i8 addrspace(1)*
----------------
sanjoy wrote:
> Can we remove the bitcasting logic from `CreateGCRelocates` and unconditionally bitcast to the type of the alloca here?  I think that will simplify the logic.
Yes, that's a good idea. In that way only this method need to be touched.

http://reviews.llvm.org/D9592

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/