This is an archive of the discontinued LLVM Phabricator instance.

[Statepoints] Refactor GCRelocateOperands into an intrinsic wrapper. NFC.
ClosedPublic

Authored by mjacob on Dec 23 2015, 5:41 PM.

Details

Summary

This commit renames GCRelocateOperands to GCRelocateInst and makes it an
intrinsic wrapper, similar to e.g. MemCpyInst. Also, all users of
GCRelocateOperands were changed to use the new intrinsic wrapper instead.

Diff Detail

Event Timeline

mjacob updated this revision to Diff 43570.Dec 23 2015, 5:41 PM
mjacob retitled this revision from to [Statepoints] Refactor GCRelocateOperands into an intrinsic wrapper. NFC..
mjacob updated this object.
mjacob added reviewers: reames, sanjoy.
mjacob added a subscriber: llvm-commits.
reames edited edge metadata.Dec 24 2015, 11:09 AM
reames added a subscriber: reames.

Won't have a chance for an actual review until next week, but wanted to say thanks for the cleanup. This is a major improvement over the current code. We probably should do the same for gc.result and gc.statepoint as well at some point.

Naming wise, I think GCRelocateInst would be more consistent with the other intrinsic wrapper classes.

Philip

Won't have a chance for an actual review until next week, but wanted to say thanks for the cleanup. This is a major improvement over the current code. We probably should do the same for gc.result and gc.statepoint as well at some point.

I'm not sure it makes sense to introduce a wrapper class for gc.result because it would probably only provide a getStatepoint() method. And gc.statepoint would require something different because it can be an invoke as well. I have something in mind which involves making Statepoint a subclass of CallSite.

Naming wise, I think GCRelocateInst would be more consistent with the other intrinsic wrapper classes.

Yes, that makes sense. I'll change it in the next version.

sanjoy edited edge metadata.Dec 24 2015, 3:58 PM

Some minor comments inline.

include/llvm/IR/Statepoint.h
386

LLVM convention is auto * to make it obvious that Relocate is a pointer.

404

Same here -- auto *Relocate is preferable.

mjacob updated this revision to Diff 43619.Dec 24 2015, 4:17 PM
mjacob edited edge metadata.

Rename GCRelocate to GCRelocateInst and address Sanjoy's comments.

mjacob updated this object.Dec 24 2015, 4:21 PM
reames accepted this revision.Jan 4 2016, 3:34 PM
reames edited edge metadata.

LGTM. Thanks again for the cleanup!

lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
1650

minor: the normal idiom is to dyn_cast, then check for null.

This revision is now accepted and ready to land.Jan 4 2016, 3:34 PM
mjacob closed this revision.Jan 4 2016, 8:06 PM
This revision was automatically updated to reflect the committed changes.