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

[Sanjoy Das]

Overall I think this is going in the right direction.

Some of the functions you touched don't have the right variable naming conventions (LLVM's coding style says camel case starting with upper case), it would be nice if you follow this change up with a renaming-only change that fixes the functions you touched.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:627
@@ +626,3 @@
+    if (RelocatedBase->getType() != Base->getType()) {
+      ActualRelocatedBase =
+          cast<Instruction>(Builder.CreateBitCast(RelocatedBase, Base->getType()));
----------------
Why not directly insert `ActualRelocatedBase` after `RelocatedBase` by something like:

```
if (...) {
  IRBuilder::InsertPointGuard IPG(RelocatedBase->getNextNode());
  Builder.CreateBitCast ...
```

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:642
@@ +641,3 @@
+    if (ReplacementInst->getType() != ToReplace->getType()) {
+      ActualReplacement =
+          cast<Instruction>(Builder.CreateBitCast(ReplacementInst, ToReplace->getType()));
----------------
Same comment as above about directly inserting the instruction in the right place.

================
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;
----------------
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.

================
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()));
+    }
----------------
Maybe have a `auto *GCRelocateType = cast<PointerType>(II->getType());` hoisted out?

================
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));
----------------
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.

================
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)*
----------------
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.

http://reviews.llvm.org/D9592

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