PlaceSafepoints: use IRBuilder helpers

[artagnon]

Summary

Use the IRBuilder helpers for gc.statepoint and gc.result, instead of
coding the construction by hand. Note that the gc.statepoint IRBuilder
handles only CallInst, not InvokeInst; retain that part of hand-coding.

[reames]

The relocate part of this change LGTM w/one change: The old code took the name of the replaced call for the result. This is useful for keeping the IR readable. Please add a test for this as well.

The statepoint parts I would prefer to delay a week or so. I'm waiting on a coworker to land the invoke specific tests and I want that to happen before adjusting the codepath in question.

[artagnon]

In D7518#120876, @reames wrote:

The relocate part of this change LGTM w/one change: The old code took the name of the replaced call for the result. This is useful for keeping the IR readable. Please add a test for this as well.

It's not what you'd naively expect, because the replaceAllUsesWith is called after the new instruction is inserted into the tree: therefore, you get %foo1 where %foo was the name of the original call. Would you like me to retain (and test) this bogus expectation, or should I retain the empty string, and attempt the name splicing in another patch (doing it the way I did it in the early CGP patch)?

[reames]

In D7518#120888, @artagnon wrote:

In D7518#120876, @reames wrote:

The relocate part of this change LGTM w/one change: The old code took the name of the replaced call for the result. This is useful for keeping the IR readable. Please add a test for this as well.

It's not what you'd naively expect, because the replaceAllUsesWith is called after the new instruction is inserted into the tree: therefore, you get %foo1 where %foo was the name of the original call. Would you like me to retain (and test) this bogus expectation, or should I retain the empty string, and attempt the name splicing in another patch (doing it the way I did it in the early CGP patch)?

Feel free to take either approach. No preference. takeName might be the easiest approach, but entirely up to you. You do not need to preserve the exact mangling behaviour.

[reames]

http://reviews.llvm.org/D7535 (the invoke tests) should land in the morning. Once that's in, feel free to submit this. No further review needed.

llvm/trunk/include/llvm/IR/IRBuilder.h

public:
   /// \brief Create a call to the experimental.gc.statepoint intrinsic to
   /// start a new statepoint sequence.
   CallInst *CreateGCStatepoint(Value *ActualCallee,
                                ArrayRef<Value *> CallArgs,
                                ArrayRef<Value *> DeoptArgs,
                                ArrayRef<Value *> GCArgs,
                                const Twine &Name = "");
 
+  // Conveninence function for the common case when CallArgs are filled in using
+  // makeArrayRef(CS.arg_begin(), .arg_end()); Use needs to be .get()'ed to get
+  // the Value *.
+  CallInst *CreateGCStatepoint(Value *ActualCallee, ArrayRef<Use> CallArgs,
+                               ArrayRef<Value *> DeoptArgs,
+                               ArrayRef<Value *> GCArgs,
+                               const Twine &Name = "");
+
   /// \brief Create a call to the experimental.gc.result intrinsic to extract
   /// the result from a call wrapped in a statepoint.
   CallInst *CreateGCResult(Instruction *Statepoint,
                            Type *ResultType,
                            const Twine &Name = "");
 
   /// \brief Create a call to the experimental.gc.relocate intrinsics to
   /// project the relocated value of one pointer from the statepoint.

llvm/trunk/lib/IR/IRBuilder.cpp

  args.insert(args.end(), CallArgs.begin(), CallArgs.end());
  args.push_back(getInt32(DeoptArgs.size()));
  args.insert(args.end(), DeoptArgs.begin(), DeoptArgs.end());
  args.insert(args.end(), GCArgs.begin(), GCArgs.end());
 
  return createCallHelper(FnStatepoint, args, this, Name);
 }
 
+CallInst *IRBuilderBase::CreateGCStatepoint(Value *ActualCallee,
+                                            ArrayRef<Use> CallArgs,
+                                            ArrayRef<Value *> DeoptArgs,
+                                            ArrayRef<Value *> GCArgs,
+                                            const Twine &Name) {
+  std::vector<Value *> VCallArgs;
+  for (auto &U : CallArgs)
+    VCallArgs.push_back(U.get());
+  return CreateGCStatepoint(ActualCallee, VCallArgs, DeoptArgs, GCArgs, Name);
+}
+
 CallInst *IRBuilderBase::CreateGCResult(Instruction *Statepoint,
                                        Type *ResultType,
                                        const Twine &Name) {
  Intrinsic::ID ID = Intrinsic::experimental_gc_result;
  Module *M = BB->getParent()->getParent();
  Type *Types[] = {ResultType};
  Value *FnGCResult = Intrinsic::getDeclaration(M, ID, Types);
 

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

static Value *ReplaceWithStatepoint(const CallSite &CS, /* to replace */
   Module *M = F->getParent();
   assert(M && "must be set");
 
   // TODO: technically, a pass is not allowed to get functions from within a
   // function pass since it might trigger a new function addition.  Refactor
   // this logic out to the initialization of the pass.  Doesn't appear to
   // matter in practice.
 
-  // Fill in the one generic type'd argument (the function is also vararg)
-  std::vector<Type *> argTypes;
-  argTypes.push_back(CS.getCalledValue()->getType());
-
-  Function *gc_statepoint_decl = Intrinsic::getDeclaration(
-      M, Intrinsic::experimental_gc_statepoint, argTypes);
-
   // Then go ahead and use the builder do actually do the inserts.  We insert
   // immediately before the previous instruction under the assumption that all
   // arguments will be available here.  We can't insert afterwards since we may
   // be replacing a terminator.
   Instruction *insertBefore = CS.getInstruction();
   IRBuilder<> Builder(insertBefore);
-  // First, create the statepoint (with all live ptrs as arguments).
-  std::vector<llvm::Value *> args;
-  // target, #call args, unused, call args..., #deopt args, deopt args..., gc args...
-  Value *Target = CS.getCalledValue();
-  args.push_back(Target);
-  int callArgSize = CS.arg_size();
-  args.push_back(
-      ConstantInt::get(Type::getInt32Ty(M->getContext()), callArgSize));
-  // TODO: add a 'Needs GC-rewrite' later flag
-  args.push_back(ConstantInt::get(Type::getInt32Ty(M->getContext()), 0));
-
-  // Copy all the arguments of the original call
-  args.insert(args.end(), CS.arg_begin(), CS.arg_end());
-
-  // # of deopt arguments: this pass currently does not support the
-  // identification of deopt arguments.  If this is interesting to you,
-  // please ask on llvm-dev.
-  args.push_back(ConstantInt::get(Type::getInt32Ty(M->getContext()), 0));
 
   // Note: The gc args are not filled in at this time, that's handled by
   // RewriteStatepointsForGC (which is currently under review).
 
   // Create the statepoint given all the arguments
   Instruction *token = nullptr;
   AttributeSet return_attributes;
   if (CS.isCall()) {
     CallInst *toReplace = cast<CallInst>(CS.getInstruction());
-    CallInst *call =
-        Builder.CreateCall(gc_statepoint_decl, args, "safepoint_token");
-    call->setTailCall(toReplace->isTailCall());
-    call->setCallingConv(toReplace->getCallingConv());
+    CallInst *Call = Builder.CreateGCStatepoint(
+        CS.getCalledValue(), makeArrayRef(CS.arg_begin(), CS.arg_end()), None,
+        None, "safepoint_token");
+    Call->setTailCall(toReplace->isTailCall());
+    Call->setCallingConv(toReplace->getCallingConv());
 
     // Before we have to worry about GC semantics, all attributes are legal
     AttributeSet new_attrs = toReplace->getAttributes();
     // In case if we can handle this set of sttributes - set up function attrs
     // directly on statepoint and return attrs later for gc_result intrinsic.
-    call->setAttributes(new_attrs.getFnAttributes());
+    Call->setAttributes(new_attrs.getFnAttributes());
     return_attributes = new_attrs.getRetAttributes();
     // TODO: handle param attributes
 
-    token = call;
+    token = Call;
 
     // Put the following gc_result and gc_relocate calls immediately after the
     // the old call (which we're about to delete)
     BasicBlock::iterator next(toReplace);
     assert(BB->end() != next && "not a terminator, must have next");
     next++;
     Instruction *IP = &*(next);
     Builder.SetInsertPoint(IP);
     Builder.SetCurrentDebugLocation(IP->getDebugLoc());
 
   } else if (CS.isInvoke()) {
+    // TODO: make CreateGCStatepoint return an Instruction that we can cast to a
+    // Call or Invoke, instead of doing this junk here.
+
+    // Fill in the one generic type'd argument (the function is also
+    // vararg)
+    std::vector<Type *> argTypes;
+    argTypes.push_back(CS.getCalledValue()->getType());
+
+    Function *gc_statepoint_decl = Intrinsic::getDeclaration(
+        M, Intrinsic::experimental_gc_statepoint, argTypes);
+
+    // First, create the statepoint (with all live ptrs as arguments).
+    std::vector<llvm::Value *> args;
+    // target, #call args, unused, ... call parameters, #deopt args, ... deopt
+    // parameters, ... gc parameters
+    Value *Target = CS.getCalledValue();
+    args.push_back(Target);
+    int callArgSize = CS.arg_size();
+    // #call args
+    args.push_back(Builder.getInt32(callArgSize));
+    // unused
+    args.push_back(Builder.getInt32(0));
+    // call parameters
+    args.insert(args.end(), CS.arg_begin(), CS.arg_end());
+    // #deopt args: 0
+    args.push_back(Builder.getInt32(0));
+
     InvokeInst *toReplace = cast<InvokeInst>(CS.getInstruction());
 
     // Insert the new invoke into the old block.  We'll remove the old one in a
     // moment at which point this will become the new terminator for the
     // original block.
     InvokeInst *invoke = InvokeInst::Create(
         gc_statepoint_decl, toReplace->getNormalDest(),
         toReplace->getUnwindDest(), args, "", toReplace->getParent());
static Value *ReplaceWithStatepoint(const CallSite &CS, /* to replace */
   }
   assert(token);
 
   // Handle the return value of the original call - update all uses to use a
   // gc_result hanging off the statepoint node we just inserted
 
   // Only add the gc_result iff there is actually a used result
   if (!CS.getType()->isVoidTy() && !CS.getInstruction()->use_empty()) {
-    Instruction *gc_result = nullptr;
-    std::vector<Type *> types;     // one per 'any' type
-    types.push_back(CS.getType()); // result type
-    Intrinsic::ID Id = Intrinsic::experimental_gc_result;
-    Value *gc_result_func = Intrinsic::getDeclaration(M, Id, types);
-
-    std::vector<Value *> args;
-    args.push_back(token);
-    gc_result = Builder.CreateCall(
-        gc_result_func, args,
-        CS.getInstruction()->hasName() ? CS.getInstruction()->getName() : "");
-
-    cast<CallInst>(gc_result)->setAttributes(return_attributes);
+    std::string takenName =
+      CS.getInstruction()->hasName() ? CS.getInstruction()->getName() : "";
+    CallInst *gc_result =
+        Builder.CreateGCResult(token, CS.getType(), takenName);
+    gc_result->setAttributes(return_attributes);
     return gc_result;
   } else {
     // No return value for the call.
     return nullptr;
   }
 }

llvm/trunk/test/Transforms/PlaceSafepoints/basic.ll

 
 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_i1i1f
 ; CHECK: (i1 (i1)* @i1_return_i1, i32 1, i32 0, i1 false, i32 0)
-; CHECK: gc.result.i1
+; CHECK: %call12 = 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()