[StatepointLowering] Remove distinction between call and invoke safepoints

[igor-laevsky]

Summary

There is no point in having invoke safepoints handled differently than the call safepoints. All relevant decisions could be made by looking at whether or not gc.result and gc.relocate lay in a same basic block. This change will allow to lower call safepoints with relocates and results in a different basic blocks. See test case for example.

[reames]

Looks straight forward. Two minor comments, but once those are addressed (in particular, the HasDef one), should be a quick LGTM.

lib/CodeGen/SelectionDAG/StatepointLowering.cpp

348

Shouldn't it be an assert that GCResult is available if HasDef? Actually, no. But I think the gc.result in basic block case is going to be encountering the no use of result case. Is that what you want?

369–370

It looks like this comment is a bit stale. Can you emphasis that this is handling a gc.result within the same basic block? i.e. nothing specific to calls vs invokes.

[igor-laevsky]

lib/CodeGen/SelectionDAG/StatepointLowering.cpp

348

Actually there is no difference on how to handle this case. I restructured code to make it more clear.

[reames]

LGTM

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

if (isa<TerminatorInst>(&I))
     HandlePHINodesInSuccessorBlocks(I.getParent());
 
   ++SDNodeOrder;
 
   CurInst = &I;
 
   visit(I.getOpcode(), I);
 
-  if (!isa<TerminatorInst>(&I) && !HasTailCall)
+  if (!isa<TerminatorInst>(&I) && !HasTailCall &&
+      !isStatepoint(&I)) // statepoints handle their exports internally
     CopyToExportRegsIfNeeded(&I);
 
   CurInst = nullptr;
 }
 
 void SelectionDAGBuilder::visitPHI(const PHINode &) {
   llvm_unreachable("SelectionDAGBuilder shouldn't visit PHI nodes!");
 }

lib/CodeGen/SelectionDAG/StatepointLowering.cpp

lowerCallFromStatepoint(ImmutableStatepoint ISP, const BasicBlock *EHPadBB,
   // slot.
 
   if (HasDef && (CallEnd->getOpcode() == ISD::CopyFromReg ||
                  CallEnd->getOpcode() == ISD::LOAD))
     CallEnd = CallEnd->getOperand(0).getNode();
 
   assert(CallEnd->getOpcode() == ISD::CALLSEQ_END && "expected!");
 
-  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 actual call. It means that standard 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.
+  // Export the result value if needed
+  const Instruction *GCResult = ISP.getGCResult();
+  if (HasDef && GCResult) {
+    if (GCResult->getParent() != CS.getParent()) {
+      // Result value will be used in a different basic block so we need to
+      // export it now.
+      // Default exporting mechanism will not work here because statepoint call
+      // has a different type than the actual call. It means that by default
+      // llvm will create export register of the wrong type (always i32 in our
+      // case). So instead we need to create export register with correct type
+      // manually.
       // TODO: To eliminate this problem we can remove gc.result intrinsics
       //       completely and make statepoint call to return a tuple.
       unsigned Reg = Builder.FuncInfo.CreateRegs(ISP.getActualReturnType());
       RegsForValue RFV(
           *Builder.DAG.getContext(), Builder.DAG.getTargetLoweringInfo(),
           Builder.DAG.getDataLayout(), Reg, ISP.getActualReturnType());
       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
+      // Result value will be used in a same basic block. Don't export it or
+      // perform any explicit register copies.
+      // We'll replace the actuall call node shortly. gc_result will grab
       // this value.
       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()));
   }
if (Loc.getNode()) {
       SpillMap[V] = None;
 
       // Default llvm mechanisms for exporting values which are used in
       // different basic blocks does not work for gc relocates.
       // Note that it would be incorrect to teach llvm that all relocates are
       // uses of the corresponding values so that it would automatically
       // export them. Relocates of the spilled values does not use original
       // value.
-      if (StatepointSite.getCallSite().isInvoke())
+      if (RelocateOpers.getUnderlyingCallSite().getParent() !=
+          StatepointInstr->getParent())
         Builder.ExportFromCurrentBlock(V);
     }
   }
 }
 
 void SelectionDAGBuilder::visitStatepoint(const CallInst &CI) {
   // Check some preconditions for sanity
   assert(isStatepoint(&CI) &&
void SelectionDAGBuilder::LowerStatepoint(
 
   NumOfStatepoints++;
   // Clear state
   StatepointLowering.startNewStatepoint(*this);
 
   ImmutableCallSite CS(ISP.getCallSite());
 
 #ifndef NDEBUG
-  // Consistency check. Don't do this for invokes. It would be too
-  // expensive to preserve this information across different basic blocks
-  if (!CS.isInvoke()) {
-    for (const User *U : CS->users()) {
-      const CallInst *Call = cast<CallInst>(U);
-      if (isGCRelocate(Call))
-        StatepointLowering.scheduleRelocCall(*Call);
-    }
-  }
+  // Consistency check. Check only relocates in the same basic block as thier
+  // statepoint.
+    if (isGCRelocate(Call) && Call->getParent() == CS.getParent())
 #endif
 
 #ifndef NDEBUG
   // If this is a malformed statepoint, report it early to simplify debugging.
   // This should catch any IR level mistake that's made when constructing or
   // transforming statepoints.
   ISP.verify();
 

 }
 
 void SelectionDAGBuilder::visitGCResult(const CallInst &CI) {
   // The result value of the gc_result is simply the result of the actual
   // call.  We've already emitted this, so just grab the value.
   Instruction *I = cast<Instruction>(CI.getArgOperand(0));
   assert(isStatepoint(I) && "first argument must be a statepoint token");
 
-  if (isa<InvokeInst>(I)) {
-    // For invokes we should have stored call result in a virtual register.
+  if (I->getParent() != CI.getParent()) {
+    // Statepoint is in different basic block so we should have stored call
+    // result in a virtual register.
     // We can not use default getValue() functionality to copy value from this
     // register because statepoint and actuall call return types can be
     // different, and getValue() will use CopyFromReg of the wrong type,
     // which is always i32 in our case.
     PointerType *CalleeType = cast<PointerType>(
         ImmutableStatepoint(I).getCalledValue()->getType());
     Type *RetTy =
         cast<FunctionType>(CalleeType->getElementType())->getReturnType();
     SDValue CopyFromReg = getCopyFromRegs(I, RetTy);
 
     assert(CopyFromReg.getNode());
     setValue(&CI, CopyFromReg);
   } else {
     setValue(&CI, getValue(I));
   }
 }
 
 void SelectionDAGBuilder::visitGCRelocate(const CallInst &CI) {
   GCRelocateOperands RelocateOpers(&CI);
 
 #ifndef NDEBUG
   // Consistency check
-  // We skip this check for invoke statepoints. It would be too expensive to
-  // preserve validation info through different basic blocks.
-  if (!RelocateOpers.isTiedToInvoke()) {
+  // We skip this check for relocates not in the same basic block as thier
+  // statepoint. It would be too expensive to preserve validation info through
+  // different basic blocks.
+  if (RelocateOpers.getStatepoint()->getParent() == CI.getParent()) {
     StatepointLowering.relocCallVisited(CI);
   }
 #endif
 
   const Value *DerivedPtr = RelocateOpers.getDerivedPtr();
   SDValue SD = getValue(DerivedPtr);
 
   FunctionLoweringInfo::StatepointSpilledValueMapTy &SpillMap =

test/CodeGen/X86/statepoint-call-lowering.ll

 ; CHECK: popq %rdx
 ; CHECK: retq
 entry:
   %safepoint_token = tail call i32 (i64, i32, i1 ()*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_i1f(i64 0, i32 3, i1 ()*null, i32 0, i32 0, i32 0, i32 0)
   %call1 = call zeroext i1 @llvm.experimental.gc.result.i1(i32 %safepoint_token)
   ret i1 %call1
 }
 
+declare void @consume(i32 addrspace(1)* %obj)
+
+define i1 @test_cross_bb(i32 addrspace(1)* %a, i1 %external_cond) gc "statepoint-example" {
+; CHECK-LABEL: test_cross_bb
+; CHECK: movq
+; CHECK: callq return_i1
+; CHECK: %left
+; CHECK: movq
+; CHECK-NEXT: callq consume
+; CHECK: retq
+entry:
+  %safepoint_token = tail call i32 (i64, i32, i1 ()*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_i1f(i64 0, i32 0, i1 ()* @return_i1, i32 0, i32 0, i32 0, i32 0, i32 addrspace(1)* %a)
+  br i1 %external_cond, label %left, label %right
+
+left:
+  %call1 = call i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(i32 %safepoint_token,  i32 7, i32 7)
+  %call2 = call zeroext i1 @llvm.experimental.gc.result.i1(i32 %safepoint_token)
+  call void @consume(i32 addrspace(1)* %call1)
+  ret i1 %call2
+
+right:
+  ret i1 true
+}
+
+
 declare i32 @llvm.experimental.gc.statepoint.p0f_i1f(i64, i32, i1 ()*, i32, i32, ...)
 declare i1 @llvm.experimental.gc.result.i1(i32)
 
 declare i32 @llvm.experimental.gc.statepoint.p0f_i32f(i64, i32, i32 ()*, i32, i32, ...)
 declare i32 @llvm.experimental.gc.result.i32(i32)
 
 declare i32 @llvm.experimental.gc.statepoint.p0f_p0i32f(i64, i32, i32* ()*, i32, i32, ...)
 declare i32* @llvm.experimental.gc.result.p0i32(i32)
 
 declare i32 @llvm.experimental.gc.statepoint.p0f_f32f(i64, i32, float ()*, i32, i32, ...)
 declare float @llvm.experimental.gc.result.f32(i32)
 
 declare i32 @llvm.experimental.gc.statepoint.p0f_isVoidi32varargf(i64, i32, void (i32, ...)*, i32, i32, ...)
 
 declare i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(i32, i32, i32)