[PlaceSafepoints] Introduce a -spp-no-statepoints flag

[sanjoy]

Summary

This change adds a -spp-no-statepoints flag to PlaceSafepoints that
bypasses the code that wraps newly introduced polls and existing calls
in gc.statepoint. With -spp-no-statepoints enabled, PlaceSafepoints
effectively becomes a safpeoint poll insertion pass.

The eventual goal is to "constant fold" this option, along with
-rs4gc-use-deopt-bundles to true, once clients using gc.statepoint
are okay doing so.

[reames]

Sanjoy, I don't see anything wrong with this patch per say, but I'm a bit confused about how you're expecting this to fit together with RS4GC. That pass currently has two modes, one which consumes statepoints, and one which consumes calls with deopt. You don't seem to be emitting either form here. What am I missing?

[sanjoy]

In D16439#333624, @reames wrote:

Sanjoy, I don't see anything wrong with this patch per say, but I'm a bit confused about how you're expecting this to fit together with RS4GC. That pass currently has two modes, one which consumes statepoints, and one which consumes calls with deopt. You don't seem to be emitting either form here. What am I missing?

That's a really good question.

For environments that don't care about deoptimization at all (which I
think includes LLILC), RS4GC can be run with
-rs4gc-allow-statepoint-with-no-deopt-info. Then RS4GC will work as
expected with calls / invokes with no "deopt" operand bundles
(i.e. the deopt section in the gc.statepoints it creates will be empty
for such calls).

Environments that do care about deoptimization and want to run
PlaceSafepoints will have to provide their own deopt info, and attach
them to all non-leaf calls including the call in the safepoint poll
slow path, if there is one. For sanity checking, in these cases we
should set -rs4gc-allow-statepoint-with-no-deopt-info to false
(unless not having deopt info for some gc.statepoint is actually a
valid thing to do in this case).

And the third possibility is to not use PlaceSafepoints at all, and
instead emit all safepoint polls early (with appropriate deopt state),
and have an optimization pass remove unnecessary polls as appropriate;
and run RS4GC afterwards. For some strange reason, this third
approach sounds very familiar. :)

Are there other possibilities that you can think of?

[reames]

In D16439#333783, @sanjoy wrote:

In D16439#333624, @reames wrote:

Sanjoy, I don't see anything wrong with this patch per say, but I'm a bit confused about how you're expecting this to fit together with RS4GC. That pass currently has two modes, one which consumes statepoints, and one which consumes calls with deopt. You don't seem to be emitting either form here. What am I missing?

That's a really good question.

For environments that don't care about deoptimization at all (which I
think includes LLILC), RS4GC can be run with
-rs4gc-allow-statepoint-with-no-deopt-info. Then RS4GC will work as
expected with calls / invokes with no "deopt" operand bundles
(i.e. the deopt section in the gc.statepoints it creates will be empty
for such calls).

Environments that do care about deoptimization and want to run
PlaceSafepoints will have to provide their own deopt info, and attach
them to all non-leaf calls including the call in the safepoint poll
slow path, if there is one. For sanity checking, in these cases we
should set -rs4gc-allow-statepoint-with-no-deopt-info to false
(unless not having deopt info for some gc.statepoint is actually a
valid thing to do in this case).

And the third possibility is to not use PlaceSafepoints at all, and
instead emit all safepoint polls early (with appropriate deopt state),
and have an optimization pass remove unnecessary polls as appropriate;
and run RS4GC afterwards. For some strange reason, this third
approach sounds very familiar. :)

Are there other possibilities that you can think of?

Hm, the proliferation of options here doesn't seem like a good thing.

What is we tried the following:

• PlaceSafepoints *only* inserts calls to poll functions. No statepoints at all. (i.e. mostly this patch, but make it unconditional)
• RS4GC excepts only calls, optionally with deopt info. We completely drop the old statepoint input case. We allow GC's to opt into a requirement that all calls have deopt operands via a predicate on the gc name (and eventually, the strategy).

I'm also wondering if we should upstream the optimize poll placement pass we have (since it shares so much with PlaceSafepoints) and gut the smarts from PlaceSafepoints. We could either have both passes call the same logic, or simply run one after the other. I'm really bothered by the code duplication here. I understand your argument (previously) that late insertion is better for folks who don't have deopt, but I'm bugged by the divergent code it has caused.

[sanjoy]

In D16439#333829, @reames wrote:

In D16439#333783, @sanjoy wrote:

In D16439#333624, @reames wrote:

Sanjoy, I don't see anything wrong with this patch per say, but I'm a bit confused about how you're expecting this to fit together with RS4GC. That pass currently has two modes, one which consumes statepoints, and one which consumes calls with deopt. You don't seem to be emitting either form here. What am I missing?

That's a really good question.

For environments that don't care about deoptimization at all (which I
think includes LLILC), RS4GC can be run with
-rs4gc-allow-statepoint-with-no-deopt-info. Then RS4GC will work as
expected with calls / invokes with no "deopt" operand bundles
(i.e. the deopt section in the gc.statepoints it creates will be empty
for such calls).

Environments that do care about deoptimization and want to run
PlaceSafepoints will have to provide their own deopt info, and attach
them to all non-leaf calls including the call in the safepoint poll
slow path, if there is one. For sanity checking, in these cases we
should set -rs4gc-allow-statepoint-with-no-deopt-info to false
(unless not having deopt info for some gc.statepoint is actually a
valid thing to do in this case).

And the third possibility is to not use PlaceSafepoints at all, and
instead emit all safepoint polls early (with appropriate deopt state),
and have an optimization pass remove unnecessary polls as appropriate;
and run RS4GC afterwards. For some strange reason, this third
approach sounds very familiar. :)

Are there other possibilities that you can think of?

Hm, the proliferation of options here doesn't seem like a good thing.

What is we tried the following:

• PlaceSafepoints *only* inserts calls to poll functions. No statepoints at all. (i.e. mostly this patch, but make it unconditional)

Yes, that's the eventual goal, as stated in the commit message. But I
want to give the LLILC developers a chance to

a. Test that this new pass pipeline works for them
b. Change the frontend to emit gc-transition arguments as operand bundles

• RS4GC excepts only calls, optionally with deopt info. We completely drop the old statepoint input case. We allow GC's to opt into a requirement that all calls have deopt operands via a predicate on the gc name (and eventually, the strategy).

+1

I'm also wondering if we should upstream the optimize poll placement pass we have (since it shares so much with PlaceSafepoints) and gut the smarts from PlaceSafepoints. We could either have both passes call the same logic, or simply run one after the other. I'm really bothered by the code duplication here. I understand your argument (previously) that late insertion is better for folks who don't have deopt, but I'm bugged by the divergent code it has caused.

I think there is a possibility to have this upstream PlaceSafepoints
and the to-be-upstream-now-downstream OptimizeSafepointPolls pass
share code i.e. despite being logically different optimization
passes, they could call into a unified set of helper routines.

[reames]

With the explanation of overall direction, LGTM.

FYI, when you get back to switching the default, you'll need to port all the tests to the new output format.

lib/Transforms/Scalar/PlaceSafepoints.cpp

670 ↗

(On Diff #45636)

I would like to see a bit more context here. In particular, we expect this to become the default, and that rs4gc will take over this responsibility.

[pgavlin]

The mechanics look good to me as well. Thanks for the edifying discussion, too :)

llvm/trunk/lib/Transforms/Scalar/PlaceSafepoints.cpp

 
 // If true, split the backedge of a loop when placing the safepoint, otherwise
 // split the latch block itself.  Both are useful to support for
 // experimentation, but in practice, it looks like splitting the backedge
 // optimizes better.
 static cl::opt<bool> SplitBackedge("spp-split-backedge", cl::Hidden,
                                    cl::init(false));
 
+// If true, don't wrap calls (the ones present in the IR, and the ones
+// introduced due to polls) in gc.statepoint.
+static cl::opt<bool> NoStatepoints("spp-no-statepoints", cl::Hidden,
+                                   cl::init(false));
+
 // Print tracing output
 static cl::opt<bool> TraceLSP("spp-trace", cl::Hidden, cl::init(false));
 
 namespace {
 
 /// An analysis pass whose purpose is to identify each of the backedges in
 /// the function which require a safepoint poll to be inserted.
 struct PlaceBackedgeSafepointsImpl : public FunctionPass {
bool PlaceSafepoints::runOnFunction(Function &F) {
   // Now that we've identified all the needed safepoint poll locations, insert
   // safepoint polls themselves.
   for (Instruction *PollLocation : PollsNeeded) {
     std::vector<CallSite> RuntimeCalls;
     InsertSafepointPoll(PollLocation, RuntimeCalls);
     ParsePointNeeded.insert(ParsePointNeeded.end(), RuntimeCalls.begin(),
                             RuntimeCalls.end());
   }
+
+  // If we've been asked to not wrap the calls with gc.statepoint, then we're
+  // done.  In the near future, this option will be "constant folded" to true,
+  // and the code below that deals with insert gc.statepoint calls will be
+  // removed.  Wrapping potentially safepointing calls in gc.statepoint will
+  // then become the responsibility of the RewriteStatepointsForGC pass.
+  if (NoStatepoints)
+    return modified;
+
   PollsNeeded.clear(); // make sure we don't accidentally use
   // The dominator tree has been invalidated by the inlining performed in the
   // above loop.  TODO: Teach the inliner how to update the dom tree?
   DT.recalculate(F);
 
   if (enableCallSafepoints(F)) {
     std::vector<CallSite> Calls;
     findCallSafepoints(F, Calls);

llvm/trunk/test/Transforms/PlaceSafepoints/no-statepoints.ll

+; RUN: opt -spp-no-statepoints -S -place-safepoints < %s | FileCheck %s
+
+define void @test() gc "statepoint-example" {
+; CHECK-LABEL: test(
+entry:
+; CHECK: entry:
+; CHECK: call void @do_safepoint()
+  br label %other
+
+other:
+; CHECK: other:
+  call void undef() "gc-leaf-function"
+; CHECK: call void @do_safepoint()
+  br label %other
+}
+
+declare void @do_safepoint()
+define void @gc.safepoint_poll() {
+  call void @do_safepoint()
+  ret void
+}