This is an archive of the discontinued LLVM Phabricator instance.

Default to generating statepoints with deopt and gc-transition bundles if needed
ClosedPublic

Authored by reames on May 27 2020, 4:26 PM.

Details

Summary

Continues from D80598.

The key point of the change is to default to using operand bundles instead of the inline length prefix argument lists for statepoint nodes. An important subtlety to note is that the presence of a bundle has semantic meaning, even if it is empty. As such, we need to make a somewhat deeper change to the interface than is first obvious.

Existing code treats statepoint deopt arguments and the deopt bundle operands differently during inlining. The former is ignored (resulting in caller state being dropped), the later is merged.

We can't preserve the old behaviour for calls with deopt fed to RS4GC and then inlining, but we can avoid the no-deopt case changing. At least in internal testing, that seem to be the important one. (I'd argue the "stop merging after RS4GC" behaviour for the former was always "unexpected", but that the behaviour for non-deopt calls actually make sense.)

Diff Detail

Event Timeline

reames created this revision.May 27 2020, 4:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 27 2020, 4:26 PM
reames updated this revision to Diff 266699.May 27 2020, 4:48 PM
reames edited the summary of this revision. (Show Details)

Make a deeper change to the interface to simplify the semantic description of the patch and avoid introducing something we'd have to fix later anyway.

(Also known as, I have the aws builder spun up, so I might as well make the change which requires a global rebuild now.)

dantrushin accepted this revision.May 28 2020, 5:33 AM

LGTM with two notes:

  • Does it makes sense to make GCArgs Optional<> too? Implementation might change later, but you won't need to change interface again.
  • Now that Deopt and Transition args are Optional<>, do we need such many CreageGCStatepointCall variants?
This revision is now accepted and ready to land.May 28 2020, 5:33 AM

LGTM with two notes:

  • Does it makes sense to make GCArgs Optional<> too? Implementation might change later, but you won't need to change interface again.

At the moment, no. Or more accurately, I haven't thought about it enough yet to be comfortable revising the interface. We can always revise later if needed.

  • Now that Deopt and Transition args are Optional<>, do we need such many CreageGCStatepointCall variants?

Good question, I'll take a separate look at this and maybe clean up some of them if it makes sense.