[Inlining][NFC] Introduce a InlineFunction that takes a Statepoint.

[sanjoy]

Summary

After this change, clients of InlineFunction can get it to inline
through calls / invokes wrapped in gc_statepoint. NFC because nothing
actually exercises this code path yet.

Depends on D10631

[sanjoy]

• update with fixed patch

[reames]

I'm not sure I like the semantic intent to this patch. It seems to imply that the caller needs to know whether a particular call site is a statepoint or not. I think that's a mistake. I would expect the details of inlining through a call site which happens to be a statepoint to be hidden entirely within InlineFunction.

[reames]

Forgot to say in my previous review, but I think you need to find a way to test this. Used code is not really NFC, it's just unused. I know you're planning on building on this with related patches, but this is big enough it should be tested on it's own.

One suggestion: You could restructure PlaceSafepoints to convert to insert statepoints for gc polls, then inline using the new functionality. Anything that exercises this is fine by me.

[sanjoy]

• have InlineSite dispatch over Statepoint and CallSite using a macro.
• make the PlaceSafepoints pass use this new functionality in InlineFunction to have a mode where it does not inline the gc.safepoint_poll function by itself, but does it via the AlwaysInliner pass.

[sanjoy]

I've merged D10633 and D10632 into D10758. Their combined size is not too large IMO, and keeping them together lets me avoid adding unneeded complexity via flags like -spp-dont-pre-inline-polls just for testing.

include/llvm/Transforms/Utils/Cloning.h

 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_TRANSFORMS_UTILS_CLONING_H
 #define LLVM_TRANSFORMS_UTILS_CLONING_H
 
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/Twine.h"
+#include "llvm/IR/Statepoint.h"
 #include "llvm/IR/ValueHandle.h"
 #include "llvm/IR/ValueMap.h"
 #include "llvm/Transforms/Utils/ValueMapper.h"
 
 namespace llvm {
 
 class Module;
 class Function;

lib/Transforms/Scalar/PlaceSafepoints.cpp

 // 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));
 
 // Print tracing output
 static cl::opt<bool> TraceLSP("spp-trace", cl::Hidden, cl::init(false));
 
+
+// If this flag is true, polls are inserted as normal "alwaysinline" calls to
+// gc.safepoint_poll that get wrapped in a statepoint by the call safepoint
+// placement logic.  The client is expected to run the AlwaysInliner pass
+// afterwards to get the desired poll logic inlined into the IR.  Once LLVM is
+// adequately good at inlining through gc_statepoint wrapped calls it is
+// expected that this will be on by default.
+//
+static cl::opt<bool> DontPreInlinePolls("spp-dont-pre-inline-polls",
+                                        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 {
   static char ID;
 
   /// The output of the pass - gives a list of each backedge (described by
if (F.hasGC()) {
     return (StatepointExampleName == FunctionGCName) ||
            (CoreCLRName == FunctionGCName);
   } else
     return false;
 }
 
 // TODO: These should become properties of the GCStrategy, possibly with
 // command line overrides.
-static bool enableEntrySafepoints(Function &F) { return !NoEntry; }
-static bool enableBackedgeSafepoints(Function &F) { return !NoBackedge; }
+static bool enableEntrySafepoints(Function &F) {
+  if (isGCSafepointPoll(F)) {
+    // The poll function itself should not get a (recursive) poll.
+    assert(DontPreInlinePolls && "otherwise we should not be asking this!");
+    return false;
+  }
+
+  return !NoEntry;
+}
+
+static bool enableBackedgeSafepoints(Function &F) {
+  if (isGCSafepointPoll(F)) {
+    // The poll function itself should not get a (recursive) poll.
+    assert(DontPreInlinePolls && "otherwise we should not be asking this!");
+    return false;
+  }
+
+  return !NoBackedge;
+}
+
 static bool enableCallSafepoints(Function &F) { return !NoCall; }
 
 // Normalize basic block to make it ready to be target of invoke statepoint.
 // Ensure that 'BB' does not have phi nodes. It may require spliting it.
 static BasicBlock *normalizeForInvokeSafepoint(BasicBlock *BB,
                                                BasicBlock *InvokeParent) {
   BasicBlock *ret = BB;
 

 
 bool PlaceSafepoints::runOnFunction(Function &F) {
   if (F.isDeclaration() || F.empty()) {
     // This is a declaration, nothing to do.  Must exit early to avoid crash in
     // dom tree calculation
     return false;
   }
 
-  if (isGCSafepointPoll(F)) {
-    // Given we're inlining this inside of safepoint poll insertion, this
-    // doesn't make any sense.  Note that we do make any contained calls
-    // parseable after we inline a poll.
+  if (isGCSafepointPoll(F) && !DontPreInlinePolls) {
+    // If we're inlining this inside of safepoint poll insertion, this doesn't
+    // make any sense.  Note that we do make any contained calls parseable after
+    // we inline a poll.
     return false;
   }
 
   if (!shouldRewriteFunction(F))
     return false;
 
   bool modified = false;
 
InsertSafepointPoll(Instruction *InsertBefore,
 
   auto *F = M->getFunction(GCSafepointPollName);
   assert(F->getType()->getElementType() ==
          FunctionType::get(Type::getVoidTy(M->getContext()), false) &&
          "gc.safepoint_poll declared with wrong type");
   assert(!F->empty() && "gc.safepoint_poll must be a non-empty function");
   CallInst *PollCall = CallInst::Create(F, "", InsertBefore);
 
+  if (DontPreInlinePolls) {
+    ParsePointsNeeded.push_back(PollCall);
+    PollCall->addAttribute(AttributeSet::FunctionIndex,
+                           Attribute::AlwaysInline);
+    return;
+  }
+
   // Record some information about the call site we're replacing
   BasicBlock::iterator before(PollCall), after(PollCall);
   bool isBegin(false);
   if (before == OrigBB->begin()) {
     isBegin = true;
   } else {
     before--;
   }

lib/Transforms/Utils/InlineFunction.cpp

bool llvm::InlineFunction(InvokeInst *II, InlineFunctionInfo &IFI,
                           bool InsertLifetime) {
   return InlineFunction(CallSite(II), IFI, InsertLifetime);
 }
 
 namespace {
 class InlineSite {
   CallSite CS;
 
+#define INLINESITE_DELEGATE_GETTER(METHOD)  do {        \
+    if (Statepoint SP = Statepoint(CS))                 \
+      return SP.METHOD;                                 \
+    return CS.METHOD;                                   \
+  } while (0)
+
   bool paramHasAttr(unsigned i, Attribute::AttrKind A) const {
-    return CS.paramHasAttr(i, A);
+    INLINESITE_DELEGATE_GETTER(paramHasAttr(i, A));
   }
 
 public:
   explicit InlineSite(CallSite CS) : CS(CS) { }
 
-  Function *getCalledFunction() const { return CS.getCalledFunction(); }
+  Function *getCalledFunction() const {
+    INLINESITE_DELEGATE_GETTER(getCalledFunction());
+  }
 
-  Function *getCaller() const { return CS.getCaller(); }
+  Function *getCaller() const {
+    INLINESITE_DELEGATE_GETTER(getCaller());
+  }
 
-  Instruction *getInstruction() const { return CS.getInstruction(); }
+  Instruction *getInstruction() const {
+    INLINESITE_DELEGATE_GETTER(getInstruction());
+  }
 
   Value *getArgument(unsigned Index) const {
-    return CS.getArgument(Index);
+    INLINESITE_DELEGATE_GETTER(getArgument(Index));
+  }
+
+  bool doesNotThrow() const {
+    INLINESITE_DELEGATE_GETTER(doesNotThrow());
+  }
+
+  unsigned arg_size() const {
+    INLINESITE_DELEGATE_GETTER(arg_size());
+  }
+
+  CallSite::arg_iterator arg_begin() const {
+    INLINESITE_DELEGATE_GETTER(arg_begin());
+  }
+
+  CallSite::arg_iterator arg_end() const {
+    INLINESITE_DELEGATE_GETTER(arg_end());
   }
 
   bool isByValArgument(unsigned ArgNo) const {
     return paramHasAttr(ArgNo + 1, Attribute::ByVal);
   }
-
-  bool doesNotThrow() const { return CS.doesNotThrow(); }
-  unsigned arg_size() const { return CS.arg_size(); }
-  CallSite::arg_iterator arg_begin() const { return CS.arg_begin(); }
-  CallSite::arg_iterator arg_end() const { return CS.arg_end(); }
 };
 }
 
 static bool InlineThroughInlineSite(InlineSite IS, InlineFunctionInfo &IFI,
                                     bool InsertLifetime, Value *&ReturnVal,
                                     bool &InlinedMustTailCalls);
 
-bool llvm::InlineFunction(CallSite CS, InlineFunctionInfo &IFI,
+static bool InlineGenericCallSite(CallSite CS, InlineFunctionInfo &IFI,
                                   bool InsertLifetime) {
   Value *ReturnValOut = nullptr;
   bool InlinedMustTailCalls = false;
   if (!InlineThroughInlineSite(InlineSite(CS), IFI, InsertLifetime,
                                ReturnValOut, InlinedMustTailCalls))
     return false;
 
   Instruction *TheCall = CS.getInstruction();
   if (!TheCall->use_empty()) {
     if (ReturnValOut == TheCall) // Can happen in unreachable code
       ReturnValOut = UndefValue::get(TheCall->getType());
     TheCall->replaceAllUsesWith(ReturnValOut);
   }
 
   BasicBlock *ContainingBlock = TheCall->getParent();
 
   // Entire block may have become unreachable.
   if (InlinedMustTailCalls && pred_empty(ContainingBlock))
     ContainingBlock->eraseFromParent();
   else
     TheCall->eraseFromParent();
 
   return true;
 }
 
+static bool InlineStatepoint(Statepoint SP, InlineFunctionInfo &IFI,
+                             bool InsertLifetime) {
+  if (SP.gc_args_begin() != SP.gc_args_begin() ||
+      SP.vm_state_begin() != SP.vm_state_end() ||
+      SP.gc_transition_args_begin() != SP.gc_transition_args_end())
+    return false;
+
+  Function *Callee = SP.getCalledFunction();
+  if (!Callee->hasGC())
+    return false;
+
+  if (StringRef(Callee->getGC()) != SP.getCaller()->getGC())
+    return false;
+
+  Value *ReturnValOut = nullptr;
+  bool InlinedMustTailCalls = false;
+  if (!InlineThroughInlineSite(InlineSite(SP.getCallSite()), IFI,
+                               InsertLifetime, ReturnValOut,
+                               InlinedMustTailCalls))
+    return false;
+
+  // TODO: add handling for unreachable code that may violate
+  // defs-dominate-uses.
+  if (CallInst *Result = cast_or_null<CallInst>(SP.getGCResult())) {
+    Result->replaceAllUsesWith(ReturnValOut);
+    Result->removeFromParent();
+  }
+
+  Instruction *StatepointInst = SP.getInstruction();
+  assert(StatepointInst->use_empty() && "gc_result should be the only use!");
+  StatepointInst->eraseFromParent();
+
+  return true;
+}
+
+bool llvm::InlineFunction(CallSite CS, InlineFunctionInfo &IFI,
+                          bool InsertLifetime) {
+  if (Statepoint SP = Statepoint(CS))
+    return InlineStatepoint(SP, IFI, InsertLifetime);
+  return InlineGenericCallSite(CS, IFI, InsertLifetime);
+}
+
 namespace {
   /// A class for recording information about inlining through an invoke.
   class InvokeInliningInfo {
     BasicBlock *OuterResumeDest; ///< Destination of the invoke's unwind.
     BasicBlock *InnerResumeDest; ///< Destination for the callee's resume.
     LandingPadInst *CallerLPad;  ///< LandingPadInst associated with the invoke.
     PHINode *InnerEHValuesPHI;   ///< PHI for EH values from landingpad insts.
     SmallVector<Value*, 8> UnwindDestPHIValues;

test/Transforms/PlaceSafepoints/basic.ll

-; RUN: opt %s -S -place-safepoints | FileCheck %s
-
+; RUN: opt %s -S -place-safepoints | FileCheck %s --check-prefix CHECK --check-prefix WITH-PRE-INLINE
+; RUN: opt %s -S -spp-dont-pre-inline-polls -place-safepoints -always-inline | FileCheck %s --check-prefix CHECK --check-prefix WITHOUT-PRE-INLINE
 
 ; Do we insert a simple entry safepoint?
 define void @test_entry() gc "statepoint-example" {
 ; CHECK-LABEL: @test_entry
 entry:
 ; CHECK-LABEL: entry
 ; CHECK: statepoint
   ret void
 }
 
 ; On a non-gc function, we should NOT get an entry safepoint
 define void @test_negative() {
 ; CHECK-LABEL: @test_negative
 entry:
-; CHECK-NOT: statepoint
+; CHECK-NOT: statepoint{{.*}}do_safepoint
   ret void
 }
 
 ; Do we insert a backedge safepoint in a statically
 ; infinite loop?
 define void @test_backedge() gc "statepoint-example" {
 ; CHECK-LABEL: test_backedge
 entry:
 ; CHECK-LABEL: entry
 ; This statepoint is technically not required, but we don't exploit that yet.
-; CHECK: statepoint
+; CHECK: statepoint{{.*}}do_safepoint
   br label %other
 
 ; CHECK-LABEL: other
 ; CHECK: statepoint
 other:
   call void undef()
   br label %other
 }
 
 ; Check that we remove an unreachable block rather than trying
 ; to insert a backedge safepoint
 define void @test_unreachable() gc "statepoint-example" {
 ; CHECK-LABEL: test_unreachable
 entry:
 ; CHECK-LABEL: entry
-; CHECK: statepoint
+; CHECK: statepoint{{.*}}do_safepoint
   ret void
 
 ; CHECK-NOT: other
-; CHECK-NOT: statepoint
+; CHECK-NOT: statepoint{{.*}}do_safepoint
 other:
   br label %other
 }
 
 declare void @foo()
 
 ; Do we turn a call into it's own statepoint
 define void @test_simple_call() gc "statepoint-example" {
 ; CHECK-LABEL: test_simple_call
 entry:
   br label %other
 other:
 ; CHECK-LABEL: other
-; CHECK: statepoint
+; CHECK: statepoint{{.*}}do_safepoint
 ; CHECK-NOT: gc.result
   call void @foo()
   ret void
 }
 
 declare zeroext i1 @i1_return_i1(i1)
 
 define i1 @test_call_with_result() gc "statepoint-example" {
 ; CHECK-LABEL: test_call_with_result
 ; This is checking that a statepoint_poll + statepoint + result is
 ; inserted for a function that takes 1 argument.
-; CHECK: gc.statepoint.p0f_isVoidf
+; CHECK: gc.statepoint.p0f_isVoidf{{.*}}do_safepoint
 ; CHECK: gc.statepoint.p0f_i1i1f
 ; CHECK: (i64 2882400000, i32 0, i1 (i1)* @i1_return_i1, i32 1, i32 0, i1 false, i32 0, i32 0)
 ; CHECK: %call1.2 = call i1 @llvm.experimental.gc.result.i1
 entry:
   %call1 = tail call i1 (i1) @i1_return_i1(i1 false)
   ret i1 %call1
 }
 
 ; This function is inlined when inserting a poll.  To avoid recursive 
 ; issues, make sure we don't place safepoints in it.
 declare void @do_safepoint()
-define void @gc.safepoint_poll() {
-; CHECK-LABEL: gc.safepoint_poll
-; CHECK-LABEL: entry
-; CHECK-NEXT: do_safepoint
-; CHECK-NEXT: ret void 
+define void @gc.safepoint_poll() gc "statepoint-example" {
+; WITH-PRE-INLINE-LABEL: gc.safepoint_poll
+; WITH-PRE-INLINE-NOT: gc.safepoint_poll
+; WITH-PRE-INLINE-LABEL: entry
+; WITH-PRE-INLINE-NOT: statepoint
+; WITH-PRE-INLINE-NEXT: do_safepoint
+; WITH-PRE-INLINE-NEXT: ret void 
+
+; WITHOUT-PRE-INLINE-LABEL: gc.safepoint_poll
+; WITHOUT-PRE-INLINE-NOT: gc.safepoint_poll
+; WITHOUT-PRE-INLINE-LABEL: entry
+; WITHOUT-PRE-INLINE: statepoint
 entry:
   call void @do_safepoint()
   ret void
 }