This is an archive of the discontinued LLVM Phabricator instance.

Add a pass for inserting safepoints into (nearly) arbitrary IR
ClosedPublic

Authored by reames on Jan 14 2015, 3:01 PM.

Details

Summary

This pass is responsible for figuring out where to place call safepoints and safepoint polls. It doesn't actually make the relocations explicit; that's the job of the RewriteStatepointsForGC pass (http://reviews.llvm.org/D6975).

Points for review:

  • I'd like to get this landed relatively quickly. This code is actually changing a bit as we work through optimization issues. For example, the SplitBackedge option was just added yesterday.
  • I welcome suggestions on how to get rid of the nasty embedded pass manager trick. I think at this point that can be replaced with a more normal analysis dependency, but I don't believe having a loop analysis pass as a dependency for a Module pass works.
  • Once landed, I plan on restructuring the statepoint rewrite to use the functions add to the IRBuilder a while back. I would prefer to do this as a separate change against TOT.
  • In our version of this pass, it also handles inserting deoptimization state for the inserted safepoints. Since that code is not yet ready to be upstreamed, I ripped it out. This may get added at a later point.
  • In the current pass, the function "gc.safepoint_poll" is treated specially but is not an intrinsic. I plan to make identifying the poll function a property of the GCStrategy at some point in the near future.
  • It's not explicit in the code, but these two patches are introducing a new state for a statepoint which looks a lot like a patchpoint. There's no a transient form which doesn't yet have the relocations explicitly represented, but does prevent reordering of memory operations. Once this is in, I need to update actually make this explicit by reserving the 'unused' argument of the statepoint as a flag, updating the docs, and making the code explicitly check for such a thing. This wasn't really planned, but once I split the two passes - which was done for other reasons - the intermediate state fell out. Just reminds us once again that we need to merge statepoints and patchpoints at some point in the not that distant future.

Future directions planned:

  • Identifying more cases where a backedge safepoint isn't required to ensure timely execution of a safepoint poll.
  • Tweaking the insertion process to generate easier to optimize IR. (For example, investigating making SplitBackedge) the default.
  • Adding opt-in flags for a GCStrategy to use this pass. Once done, add this pass to the actual pass ordering.

Diff Detail

Repository
rL LLVM

Event Timeline

reames updated this revision to Diff 18188.Jan 14 2015, 3:01 PM
reames retitled this revision from to Add a pass for inserting safepoints into (nearly) arbitrary IR.
reames updated this object.
reames edited the test plan for this revision. (Show Details)
reames added reviewers: whitequark, sanjoy, atrick, ributzka.
reames added a subscriber: Unknown Object (MLST).

ping x2

(I would really appreciate a LGTM so that we can iterate in tree.)

whitequark resigned from this revision.Jan 29 2015, 3:08 PM
whitequark removed a reviewer: whitequark.

I am unfortunately not qualified to review this patch. (Same goes for D6975)

atrick accepted this revision.Feb 2 2015, 11:13 AM
atrick edited edge metadata.

In general, the code is structured well and easy to follow.

The most confusing aspect is the on-the-fly pass manager instantiation. It's not clear why you can't do all your "analysis" up front and just keep a side table keyed off LoopInfo. Then you shouldn't need to recompute Dominators, except maybe for assertions. LoopSimplify will just run on all loops before your pass begins. SCEV should not be managed with the pass manager anyway.

From reading the comments in patch, the difference between "parse
point" and "statepoint" is unclear. (You may have explained it
elsewhere). I gather that "parse point" is an abstract runtime feature, and "statepoint" is the implementation of them via LLVM intrinsic?

You may want to run clang-format before checkin. It caught a few issues.

A comment isn't the best place for a question ;)

+ // Why the hell is inline ASM modeled as a call instruction?

<sp> substaintially

lib/Transforms/Scalar/PlaceSafepoints.cpp:217: suprisingly -> "surprisingly"
lib/Transforms/Scalar/PlaceSafepoints.cpp:218: occurances -> "occurrences"
lib/Transforms/Scalar/PlaceSafepoints.cpp:404: intial -> "initial"

I'm fine with getting this in-tree and iterating.

This revision is now accepted and ready to land.Feb 2 2015, 11:13 AM
This revision was automatically updated to reflect the committed changes.