This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by chenli on May 7 2015, 5:48 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

chenli updated this revision to Diff 25267.May 7 2015, 5:48 PM
chenli retitled this revision from to [RewriteStatepointsForGC] Fix a bug on creating gc_relocate for pointer to vector of pointers .
chenli updated this object.
chenli edited the test plan for this revision. (Show Details)
chenli added reviewers: sanjoy, reames, AndyAyers.
chenli added a subscriber: Unknown Object (MLST).
sanjoy edited edge metadata.May 8 2015, 12:58 AM

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.

lib/CodeGen/CodeGenPrepare.cpp
627 ↗(On Diff #25267)

Why not directly insert ActualRelocatedBase after RelocatedBase by something like:

if (...) {
  IRBuilder::InsertPointGuard IPG(RelocatedBase->getNextNode());
  Builder.CreateBitCast ...
642 ↗(On Diff #25267)

Same comment as above about directly inserting the instruction in the right place.

lib/IR/Verifier.cpp
3365 ↗(On Diff #25267)

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.

lib/Transforms/InstCombine/InstCombineCalls.cpp
1212 ↗(On Diff #25267)

Maybe have a auto *GCRelocateType = cast<PointerType>(II->getType()); hoisted out?

lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
1080 ↗(On Diff #25267)

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.

1357 ↗(On Diff #25267)

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.

chenli added inline comments.May 8 2015, 10:00 AM
lib/CodeGen/CodeGenPrepare.cpp
627 ↗(On Diff #25267)

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.

lib/IR/Verifier.cpp
3365 ↗(On Diff #25267)

I think we should do it if it follows LLVM's direction.

lib/Transforms/InstCombine/InstCombineCalls.cpp
1212 ↗(On Diff #25267)

Will do

lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
1080 ↗(On Diff #25267)

That would make sense.

1357 ↗(On Diff #25267)

Yes, that's a good idea. In that way only this method need to be touched.

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.

I will do a follow up patch to fix names in these functions.

sanjoy accepted this revision.May 8 2015, 11:20 AM
sanjoy edited edge metadata.

LGTM

lib/CodeGen/CodeGenPrepare.cpp
627 ↗(On Diff #25267)

Sure, I think fixing all of this together in a separate change is a good idea. The Create->Remove->ReInsert flow seems very odd to me, unless there's a reason for it I can't see.

This revision is now accepted and ready to land.May 8 2015, 11:20 AM
chenli updated this revision to Diff 25358.May 8 2015, 12:18 PM
chenli edited edge metadata.

Update w.r.t Sanjoy's comments.

  1. Hoist gc_relocate type in InstCombineCalls.cpp
  2. Move bitcast logic into insertRelocationStores() in RewriteStatepointsForGC.cpp
chenli updated this revision to Diff 25370.May 8 2015, 2:24 PM
chenli updated this object.

Fix some merge conflicts.

This revision was automatically updated to reflect the committed changes.