[StatepointLowering] Don't create temporary instructions. NFCI.

[sanjoy]

Summary

Instead of creating a temporary call instruction and lowering that, use
SelectionDAGBuilder::lowerCallOperands.

[reames]

Meta comment: I can't tell if this code is wrong, but it's not obviously correct. Your mixing renames, code motion, and restructuring unnecessarily. This makes it much harder to review for correctness.

lib/CodeGen/SelectionDAG/StatepointLowering.cpp

225 ↗

(On Diff #24895)

To simplify review, please place the new code into the same helper. (I think the helper should be there regardless, but it also reduces the diff.)

526 ↗

(On Diff #24895)

This block looks like code which could and should be commoned into a helper function for statepoints and patchpoints. possible: findCallNode?

537 ↗

(On Diff #24895)

I would prefer the glue node changes go in separately. You do not need further review for them, but they're unrelated to the tricky part of this review and should be separated.

571 ↗

(On Diff #24895)

Use getGluedNode please.

580 ↗

(On Diff #24895)

You're missing a check on whether the underlying call produces a value.

591 ↗

(On Diff #24895)

I'm pretty sure using the entry point of the chain here is wrong. I don't understand the motivation for this change. Can you please separate or revert this part?

[sanjoy]

lib/CodeGen/SelectionDAG/StatepointLowering.cpp

225 ↗

(On Diff #24895)

will do.

526 ↗

(On Diff #24895)

I'll do that once this is in, if you're okay with that.

537 ↗

(On Diff #24895)

Will do.

571 ↗

(On Diff #24895)

Sure.

580 ↗

(On Diff #24895)

Yes. I wonder why this didn't break any tests.

591 ↗

(On Diff #24895)

The node copying the return value to a virtual register has a data dependence on the call, and it can thus be scheduled anywhere after the call and before the terminator instruction, but otherwise has no control dependencies. Adding it to PendingExports takes care of the latter.

SelectionDAGBuilder::CopyValueToVirtualRegister does the same thing.

[sanjoy]

This reduces the non-semantic noise in the change by keeping most of
the tricky parts of the change inside lowerCallFromStatepoint.

[reames]

LGTM w/the requirement that you write a test which would have failed with your previous version and include it with the submission.

lib/CodeGen/SelectionDAG/StatepointLowering.cpp

281 ↗

(On Diff #24996)

I would prefer you kept this comment in the new code. I regularly use it to remind myself what's going on here.

[pgavlin]

llvm/trunk/lib/CodeGen/SelectionDAG/StatepointLowering.cpp

277

Can SelectionDAGBuilder::removeValue be removed now that this code no longer uses it? AFAICT this is the only user of that method (indeed, a comment inside that method indicates as much).

llvm/trunk/include/llvm/IR/Statepoint.h

public:
   CallSiteTy getCallSite() {
     return StatepointCS;
   }
 
   /// Return the value actually being called or invoked.
   ValueTy *actualCallee() {
     return StatepointCS.getArgument(0);
   }
+  /// Return the type of the value returned by the call underlying the
+  /// statepoint.
+  Type *actualReturnType() {
+    auto *FTy = cast<FunctionType>(
+        cast<PointerType>(actualCallee()->getType())->getElementType());
+    return FTy->getReturnType();
+  }
   /// Number of arguments to be passed to the actual callee.
   int numCallArgs() {
     return cast<ConstantInt>(StatepointCS.getArgument(1))->getZExtValue();
   }
   /// Number of additional arguments excluding those intended
   /// for garbage collection.
   int numTotalVMSArgs() {
     return cast<ConstantInt>(StatepointCS.getArgument(3 + numCallArgs()))->getZExtValue();
   }
 
+  int callArgsBeginOffset() { return 3; }
+
   typename CallSiteTy::arg_iterator call_args_begin() {
     // 3 = callTarget, #callArgs, flag
-    int Offset = 3;
+    int Offset = callArgsBeginOffset();
     assert(Offset <= (int)StatepointCS.arg_size());
     return StatepointCS.arg_begin() + Offset;
   }
   typename CallSiteTy::arg_iterator call_args_end() {
-    int Offset = 3 + numCallArgs();
+    int Offset = callArgsBeginOffset() + numCallArgs();
     assert(Offset <= (int)StatepointCS.arg_size());
     return StatepointCS.arg_begin() + Offset;
   }
 
   /// range adapter for call arguments
   iterator_range<arg_iterator> call_args() {
     return iterator_range<arg_iterator>(call_args_begin(), call_args_end());
   }

llvm/trunk/lib/CodeGen/SelectionDAG/StatepointLowering.cpp

static void removeDuplicatesGCPtrs(SmallVectorImpl<const Value *> &Bases,
   Relocs = NewRelocs;
   assert(Ptrs.size() == Bases.size());
   assert(Ptrs.size() == Relocs.size());
 }
 
 /// Extract call from statepoint, lower it and return pointer to the
 /// call node. Also update NodeMap so that getValue(statepoint) will
 /// reference lowered call result
-static SDNode *lowerCallFromStatepoint(ImmutableStatepoint StatepointSite,
-                                       MachineBasicBlock *LandingPad,
-                                       SelectionDAGBuilder &Builder) {
+static SDNode *
+lowerCallFromStatepoint(ImmutableStatepoint ISP, MachineBasicBlock *LandingPad,
+                        SelectionDAGBuilder &Builder,
+                        SmallVectorImpl<SDValue> &PendingExports) {
+
+  ImmutableCallSite CS(ISP.getCallSite());
+
+  SDValue ActualCallee = Builder.getValue(ISP.actualCallee());
+
+  // Handle immediate and symbolic callees.
+  if (auto *ConstCallee = dyn_cast<ConstantSDNode>(ActualCallee.getNode()))
+    ActualCallee = Builder.DAG.getIntPtrConstant(ConstCallee->getZExtValue(),
+                                                 Builder.getCurSDLoc(),
+                                                 /*isTarget=*/true);
+  else if (auto *SymbolicCallee =
+               dyn_cast<GlobalAddressSDNode>(ActualCallee.getNode()))
+    ActualCallee = Builder.DAG.getTargetGlobalAddress(
+        SymbolicCallee->getGlobal(), SDLoc(SymbolicCallee),
+        SymbolicCallee->getValueType(0));
+
+  assert(CS.getCallingConv() != CallingConv::AnyReg &&
+         "anyregcc is not supported on statepoints!");
+
+  Type *DefTy = ISP.actualReturnType();
+  bool HasDef = !DefTy->isVoidTy();
+
+  SDValue ReturnValue, CallEndVal;
+  std::tie(ReturnValue, CallEndVal) = Builder.lowerCallOperands(
+      ISP.getCallSite(), ISP.callArgsBeginOffset(), ISP.numCallArgs(),
+      ActualCallee, DefTy, LandingPad, false /* IsPatchPoint */);
+
+  SDNode *CallEnd = CallEndVal.getNode();
+
+  // Get a call instruction from the call sequence chain.  Tail calls are not
+  // allowed.  The following code is essentially reverse engineering X86's
+  // LowerCallTo.
+  //
+  // We are expecting DAG to have the following form:
+  //
+  // ch = eh_label (only in case of invoke statepoint)
+  //   ch, glue = callseq_start ch
+  //   ch, glue = X86::Call ch, glue
+  //   ch, glue = callseq_end ch, glue
+  //   get_return_value ch, glue
+  //
+  // get_return_value can either be a CopyFromReg to grab the return value from
+  // %RAX, or it can be a LOAD to load a value returned by reference via a stack
+  // slot.
 
-  ImmutableCallSite CS(StatepointSite.getCallSite());
+  if (HasDef && (CallEnd->getOpcode() == ISD::CopyFromReg ||
+                 CallEnd->getOpcode() == ISD::LOAD))
+    CallEnd = CallEnd->getOperand(0).getNode();
 
-  // Lower the actual call itself - This is a bit of a hack, but we want to
-  // avoid modifying the actual lowering code.  This is similiar in intent to
-  // the LowerCallOperands mechanism used by PATCHPOINT, but is structured
-  // differently.  Hopefully, this is slightly more robust w.r.t. calling
-  // convention, return values, and other function attributes.
-  Value *ActualCallee = const_cast<Value *>(StatepointSite.actualCallee());
-
-  std::vector<Value *> Args;
-  CallInst::const_op_iterator arg_begin = StatepointSite.call_args_begin();
-  CallInst::const_op_iterator arg_end = StatepointSite.call_args_end();
-  Args.insert(Args.end(), arg_begin, arg_end);
-  // TODO: remove the creation of a new instruction!  We should not be
-  // modifying the IR (even temporarily) at this point.
-  CallInst *Tmp = CallInst::Create(ActualCallee, Args);
-  Tmp->setTailCall(CS.isTailCall());
-  Tmp->setCallingConv(CS.getCallingConv());
-  Tmp->setAttributes(CS.getAttributes());
-  Builder.LowerCallTo(Tmp, Builder.getValue(ActualCallee), false, LandingPad);
+  assert(CallEnd->getOpcode() == ISD::CALLSEQ_END && "expected!");
 
-  // Handle the return value of the call iff any.
-  const bool HasDef = !Tmp->getType()->isVoidTy();
   if (HasDef) {
     if (CS.isInvoke()) {
       // Result value will be used in different basic block for invokes
       // so we need to export it now. But statepoint call has a different type
       // than the actuall call. It means that standart exporting mechanism will
       // create register of the wrong type. So instead we need to create
       // register with correct type and save value into it manually.
       // TODO: To eliminate this problem we can remove gc.result intrinsics
       //       completelly and make statepoint call to return a tuple.
-      unsigned reg = Builder.FuncInfo.CreateRegs(Tmp->getType());
-      Builder.CopyValueToVirtualRegister(Tmp, reg);
-      Builder.FuncInfo.ValueMap[CS.getInstruction()] = reg;
+      unsigned Reg = Builder.FuncInfo.CreateRegs(ISP.actualReturnType());
+      RegsForValue RFV(*Builder.DAG.getContext(),
+                       Builder.DAG.getTargetLoweringInfo(), Reg,
+                       ISP.actualReturnType());
+      SDValue Chain = Builder.DAG.getEntryNode();
+
+      RFV.getCopyToRegs(ReturnValue, Builder.DAG, Builder.getCurSDLoc(), Chain,
+                        nullptr);
+      PendingExports.push_back(Chain);
+      Builder.FuncInfo.ValueMap[CS.getInstruction()] = Reg;
     } else {
       // The value of the statepoint itself will be the value of call itself.
       // We'll replace the actually call node shortly.  gc_result will grab
       // this value.
-      Builder.setValue(CS.getInstruction(), Builder.getValue(Tmp));
+      Builder.setValue(CS.getInstruction(), ReturnValue);
     }
   } else {
     // The token value is never used from here on, just generate a poison value
     Builder.setValue(CS.getInstruction(),
                      Builder.DAG.getIntPtrConstant(-1, Builder.getCurSDLoc()));
   }
-  // Remove the fake entry we created so we don't have a hanging reference
-  // after we delete this node.
-  Builder.removeValue(Tmp);
-  delete Tmp;
-  Tmp = nullptr;
-
-  // Search for the call node
-  // The following code is essentially reverse engineering X86's
-  // LowerCallTo.
-  // We are expecting DAG to have the following form:
-  // ch = eh_label (only in case of invoke statepoint)
-  //   ch, glue = callseq_start ch
-  //   ch, glue = X86::Call ch, glue
-  //   ch, glue = callseq_end ch, glue
-  // ch = eh_label ch (only in case of invoke statepoint)
-  //
-  // DAG root will be either last eh_label or callseq_end.
-
-  SDNode *CallNode = nullptr;
-
-  // We just emitted a call, so it should be last thing generated
-  SDValue Chain = Builder.DAG.getRoot();
 
-  // Find closest CALLSEQ_END walking back through lowered nodes if needed
-  SDNode *CallEnd = Chain.getNode();
-  int Sanity = 0;
-  while (CallEnd->getOpcode() != ISD::CALLSEQ_END) {
-    assert(CallEnd->getNumOperands() >= 1 &&
-           CallEnd->getOperand(0).getValueType() == MVT::Other);
-
-    CallEnd = CallEnd->getOperand(0).getNode();
-
-    assert(Sanity < 20 && "should have found call end already");
-    Sanity++;
-  }
-  assert(CallEnd->getOpcode() == ISD::CALLSEQ_END &&
-         "Expected a callseq node.");
-  assert(CallEnd->getGluedNode());
-
-  // Step back inside the CALLSEQ
-  CallNode = CallEnd->getGluedNode();
-  return CallNode;
+  return CallEnd->getOperand(0).getNode();
 }
 
 /// Callect all gc pointers coming into statepoint intrinsic, clean them up,
 /// and return two arrays:
 ///   Bases - base pointers incoming to this statepoint
 ///   Ptrs - derived pointers incoming to this statepoint
 ///   Relocs - the gc_relocate corresponding to each base/ptr pair
 /// Elements of this arrays should be in one-to-one correspondence with each
assert(GFI->getStrategy().useStatepoints() &&
          "GCStrategy does not expect to encounter statepoints");
 #endif
 
   // Lower statepoint vmstate and gcstate arguments
   SmallVector<SDValue, 10> LoweredMetaArgs;
   lowerStatepointMetaArgs(LoweredMetaArgs, ISP, *this);
 
   // Get call node, we will replace it later with statepoint
-  SDNode *CallNode = lowerCallFromStatepoint(ISP, LandingPad, *this);
+  SDNode *CallNode =
+      lowerCallFromStatepoint(ISP, LandingPad, *this, PendingExports);
 
   // Construct the actual STATEPOINT node with all the appropriate arguments
   // and return values.
 
   // TODO: Currently, all of these operands are being marked as read/write in
   // PrologEpilougeInserter.cpp, we should special case the VMState arguments
   // and flags to be read-only.
   SmallVector<SDValue, 40> Ops;

llvm/trunk/test/CodeGen/X86/statepoint-indirect-return.ll

+; RUN: llc < %s | FileCheck %s
+
+declare i1024 @g()
+
+define i1024 @f() gc "statepoint-example" {
+; CHECK-LABEL: _f
+; CHECK: callq _g
+  %1 = invoke i32 (i1024 ()*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_i1024f(i1024 ()* @g, i32 0, i32 0, i32 0)
+          to label %normal unwind label %except
+
+normal:                                           ; preds = %0
+  %x1 = call i1024 @llvm.experimental.gc.result.i1024(i32 %1)
+  ret i1024 %x1
+
+except:                                           ; preds = %0
+  %landing_pad = landingpad { i8*, i32 } personality i32 ()* @personality_function
+          cleanup
+  ret i1024 0
+}
+
+declare i32 @personality_function()
+
+; Function Attrs: nounwind
+declare i32 @llvm.experimental.gc.statepoint.p0f_i1024f(i1024 ()*, i32, i32, ...) #0
+
+; Function Attrs: nounwind
+declare i1024 @llvm.experimental.gc.result.i1024(i32) #0
+
+attributes #0 = { nounwind }