This is an archive of the discontinued LLVM Phabricator instance.

Introduce a "gc-live" bundle for the gc arguments of a statepoint
ClosedPublic

Authored by reames on Jun 1 2020, 10:22 AM.

Details

Summary

Currently, gc.relocates are defined in terms of indices into the statepoint's operand list. Given the gc args are at the end of a variable length list of operands, this makes interpreting their indices by hand a tad challenging. We can simplify the statepoint sequence and improve readability quite a bit by pulling these new operands into their own named operand bundle.

This patch defines a new operand bundle tag "gc-live". The semantics of the bundle are the same as the existing gc arguments of a statepoint. This patch simply introduces the definition and codegen for the bundle, future patches will migrate RS4GC to emitting the new form.

I'll note that I'm not particularly attached to the name "gc-live". If anyone has a better suggestion, please make it.

Interestingly, with this done and the recent migration to using deopt and gc-transition bundles, we really don't have much left in the statepoint itself. It really looks like the existing ID and flags fields are redundant; we have (existing!) attributes for all of them. I think we'll be able to reduce the gc.statepoint signature to simply a wrapped call (e.g. actual target and actual arguments).

Diff Detail

Event Timeline

reames created this revision.Jun 1 2020, 10:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2020, 10:22 AM
dantrushin accepted this revision.Jun 2 2020, 12:16 AM

LGTM with one note (you can ignore it if you have reasons).

llvm/lib/IR/Verifier.cpp
4760

BTW, I find this inconsistent with definition of GCRelocateInst.
Its base/derived ptr indices are unsigned and one can successfully create gc.relocate with index in [0, 2^32-1] range.
Then verifier suddenly shrinks allowed range into [0, 2^31-1].
Do you really want enforce index be positive signed int? Why not just require it is smaller (unsigned) than Inputs.size()?

This revision is now accepted and ready to land.Jun 2 2020, 12:16 AM
anna added a comment.Jun 2 2020, 10:05 AM

LGTM w/ comments inline.

llvm/docs/Statepoints.rst
620

Nit: then *both arguments* are indices into the operand bundle's operand.

636

or within the operand list in gc-live bundle.

llvm/test/CodeGen/X86/statepoint-gc-live.ll
39

Perhaps add a testcase for a relocated derived pointer?

anna accepted this revision.Jun 2 2020, 10:06 AM
reames marked an inline comment as done.Jun 3 2020, 3:11 PM
reames added inline comments.
llvm/lib/IR/Verifier.cpp
4760

Addressed in a separate commit.

commit ff529e0f2792e1383a602e4b8a466337fd0c2926