Verification of invoke statepoints

[igor-laevsky]

Summary

This changes implements invoke statepoint verification. It depends on the change in Statepoint.h (http://reviews.llvm.org/D7364) as it uses some of the helper functions form there.

[reames]

Comments inline.

lib/IR/Verifier.cpp

1862

Add a TODO here. We'd really should be using visitIntrinsicFunctionCall here. So should patchpoint. It just happens that's phrased in CallInst and not worth updating.

2369

Given the structure of this clause, I'd prefer to see this as a test against the intrinsic id directly.

2770

IsLandingpad doesn't actually check this is a landing pad? Possibly IsTiedToInvoke? (better: see comment below about organization)

Meta comments:

• I really don't like this organization. I'd suggest:

// structural checks only...
if (isa<IntrinsicInst>(arg_0)) {

// call statepoint case

} else if (isa<ExtractElementInst>()) {

// invoke

} else { report error }
Given the statepoint..
rest of code

• Your adding usage of the statepoint utility function in places which are deliberately not using them. (i.e. that want to report more fine grained errors)

2770

Please don't use statepoint here.

test/Verifier/statepoint.ll

55

add: gc "statepoint-example"

69

Does this extra branch add anything to this test?

[reames]

lib/IR/Verifier.cpp

2772

I'd suggest using dyn_cast here. It will simplify several parts of the following code.

2787

I think this is checked elsewhere.

2798

The cast needs to be check in a way you don't crash here. :)

auto Op0 = ...
isa<Instruction>(Op0) && ...

2806

This can be more cleanly written as:
ImmutableCallSite StatepointCS(ops.statepoint());

[reames]

LGTM, would you like me to commit on your behalf?

[igor-laevsky]

Yes, please. Please note that this patch depends on http://reviews.llvm.org/D7364

lib/IR/Verifier.cpp

2770

In this case it's fine to use ops.statepoint because we already verified that gc relocate is correctly tied to the statepoint. I made this more clear in the updated change.

2787

It is checked at the visitBasicBlock, but I didn't want to rely on the basic block visiting order.

lib/IR/Verifier.cpp

 void Verifier::visitInvokeInst(InvokeInst &II) {
   VerifyCallSite(&II);
 
   // Verify that there is a landingpad instruction as the first non-PHI
   // instruction of the 'unwind' destination.
   Assert1(II.getUnwindDest()->isLandingPad(),
           "The unwind destination does not have a landingpad instruction!",&II);
 
+  if (Function *F = II.getCalledFunction())
+    // TODO: Ideally we should use visitIntrinsicFunction here. But it uses
+    //       CallInst as an input parameter. It not woth updating this whole
+    //       function only to support statepoint verification.
+    if (F->getIntrinsicID() == Intrinsic::experimental_gc_statepoint)
+      VerifyStatepoint(ImmutableCallSite(&II));
+
   visitTerminatorInst(II);
 }
 
 /// visitBinaryOperator - Check that both arguments to the binary operator are
 /// of the same type!
 ///
 void Verifier::visitBinaryOperator(BinaryOperator &B) {
   Assert1(B.getOperand(0)->getType() == B.getOperand(1)->getType(),
if (Function *F = dyn_cast<Function>(I.getOperand(i))) {
       // Check to make sure that the "address of" an intrinsic function is never
       // taken.
       Assert1(!F->isIntrinsic() || i == (isa<CallInst>(I) ? e-1 :
                                          isa<InvokeInst>(I) ? e-3 : 0),
               "Cannot take the address of an intrinsic!", &I);
       Assert1(!F->isIntrinsic() || isa<CallInst>(I) ||
               F->getIntrinsicID() == Intrinsic::donothing ||
               F->getIntrinsicID() == Intrinsic::experimental_patchpoint_void ||
-              F->getIntrinsicID() == Intrinsic::experimental_patchpoint_i64,
+              F->getIntrinsicID() == Intrinsic::experimental_patchpoint_i64 ||
+              F->getIntrinsicID() == Intrinsic::experimental_gc_statepoint,
               "Cannot invoke an intrinsinc other than"
               " donothing or patchpoint", &I);
       Assert1(F->getParent() == M, "Referencing function in another module!",
               &I);
     } else if (BasicBlock *OpBB = dyn_cast<BasicBlock>(I.getOperand(i))) {
       Assert1(OpBB->getParent() == BB->getParent(),
               "Referring to a basic block in another function!", &I);
     } else if (Argument *OpArg = dyn_cast<Argument>(I.getOperand(i))) {
case Intrinsic::experimental_gc_result: {
     const FunctionType *TargetFuncType =
       cast<FunctionType>(PT->getElementType());
     Assert1(CI.getType() == TargetFuncType->getReturnType(),
             "gc.result result type does not match wrapped callee",
             &CI);
     break;
   }
   case Intrinsic::experimental_gc_relocate: {
-    // Are we tied to a statepoint properly?
-    CallSite StatepointCS(CI.getArgOperand(0));
-    const Function *StatepointFn =
-        StatepointCS.getInstruction() ? StatepointCS.getCalledFunction() : nullptr;
-    Assert2(StatepointFn && StatepointFn->isDeclaration() &&
-            StatepointFn->getIntrinsicID() == Intrinsic::experimental_gc_statepoint,
-            "gc.relocate operand #1 must be from a statepoint",
-      &CI, CI.getArgOperand(0));
+    Assert1(CI.getNumArgOperands() == 3, "wrong number of arguments", &CI);
+
+    // Check that this relocate is correctly tied to the statepoint
+
+    // This is case for relocate on the unwinding path of an invoke statepoint
+    if (isa<ExtractValueInst>(CI.getArgOperand(0))) {
+      Assert1(isa<LandingPadInst>(
+                cast<ExtractValueInst>(
+                  CI.getArgOperand(0))->getAggregateOperand()),
+              "gc relocate on unwind path incorrectly linked to the statepoint",
+              &CI);
+
+      const BasicBlock *invokeBB =
+        cast<Instruction>(CI.getArgOperand(0))->getParent()->getUniquePredecessor();
+
+      // Landingpad relocates should have only one predecessor with invoke
+      // statepoint terminator
+      Assert1(invokeBB,
+              "safepoints should have unique landingpads",
+              cast<Instruction>(CI.getArgOperand(0))->getParent());
+      Assert1(invokeBB->getTerminator(),
+              "safepoint block should be well formed",
+              invokeBB);
+      Assert1(isStatepoint(invokeBB->getTerminator()),
+              "gc relocate should be linked to a statepoint",
+              invokeBB);
+    }
+    else {
+      // In all other cases relocate should be tied to the statepoint directly.
+      // This covers relocates on a normal return path of invoke statepoint and
+      // relocates of a call statepoint
+      Assert2(isStatepoint(cast<Instruction>(CI.getArgOperand(0))),
+              "gc relocate is incorrectly tied to the statepoint",
+              &CI, CI.getArgOperand(0));
+    }
+
+    // Verify rest of the relocate arguments
+
+    GCRelocateOperands ops(&CI);
+    ImmutableCallSite StatepointCS = ImmutableCallSite(ops.statepoint());
 
     // Both the base and derived must be piped through the safepoint
     Value* Base = CI.getArgOperand(1);
     Assert1(isa<ConstantInt>(Base),
             "gc.relocate operand #2 must be integer offset", &CI);
     
     Value* Derived = CI.getArgOperand(2);
     Assert1(isa<ConstantInt>(Derived),

test/Verifier/statepoint.ll

 ; RUN: opt -S %s -verify | FileCheck %s
 
 declare void @use(...)
 declare i8 addrspace(1)* @llvm.experimental.gc.relocate.p1i8(i32, i32, i32)
 declare i64 addrspace(1)* @llvm.experimental.gc.relocate.p1i64(i32, i32, i32)
 declare i32 @llvm.experimental.gc.statepoint.p0f_isVoidf(void ()*, i32, i32, ...)
 declare i32 @"personality_function"()
 
 ;; Basic usage
-define i64 addrspace(1)* @test1(i8 addrspace(1)* %arg) {
+define i64 addrspace(1)* @test1(i8 addrspace(1)* %arg) gc "statepoint-example" {
 entry:
   %cast = bitcast i8 addrspace(1)* %arg to i64 addrspace(1)*
   %safepoint_token = call i32 (void ()*, i32, i32, ...)* @llvm.experimental.gc.statepoint.p0f_isVoidf(void ()* undef, i32 0, i32 0, i32 5, i32 0, i32 0, i32 0, i32 10, i32 0, i8 addrspace(1)* %arg, i64 addrspace(1)* %cast, i8 addrspace(1)* %arg, i8 addrspace(1)* %arg)
   %reloc = call i64 addrspace(1)* @llvm.experimental.gc.relocate.p1i64(i32 %safepoint_token, i32 9, i32 10)
   ;; It is perfectly legal to relocate the same value multiple times...
   %reloc2 = call i64 addrspace(1)* @llvm.experimental.gc.relocate.p1i64(i32 %safepoint_token, i32 9, i32 10)
   %reloc3 = call i8 addrspace(1)* @llvm.experimental.gc.relocate.p1i8(i32 %safepoint_token, i32 10, i32 9)
   ret i64 addrspace(1)* %reloc
 ; CHECK-LABEL: test1
 ; CHECK: statepoint
 ; CHECK: gc.relocate
 ; CHECK: gc.relocate
 ; CHECK: gc.relocate
 ; CHECK: ret i64 addrspace(1)* %reloc
 }
 
 ; This test catches two cases where the verifier was too strict:
 ; 1) A base doesn't need to be relocated if it's never used again
 ; 2) A value can be replaced by one which is known equal.  This
 ; means a potentially derived pointer can be known base and that
 ; we can't check that derived pointer are never bases.
-define void @test2(i8 addrspace(1)* %arg, i64 addrspace(1)* %arg2) {
+define void @test2(i8 addrspace(1)* %arg, i64 addrspace(1)* %arg2) gc "statepoint-example" {
 entry:
   %cast = bitcast i8 addrspace(1)* %arg to i64 addrspace(1)*
   %c = icmp eq i64 addrspace(1)* %cast,  %arg2
   br i1 %c, label %equal, label %notequal
 
 notequal:
   ret void
 
 equal:
   %safepoint_token = call i32 (void ()*, i32, i32, ...)* @llvm.experimental.gc.statepoint.p0f_isVoidf(void ()* undef, i32 0, i32 0, i32 5, i32 0, i32 0, i32 0, i32 10, i32 0, i8 addrspace(1)* %arg, i64 addrspace(1)* %cast, i8 addrspace(1)* %arg, i8 addrspace(1)* %arg)
   %reloc = call i64 addrspace(1)* @llvm.experimental.gc.relocate.p1i64(i32 %safepoint_token, i32 9, i32 10)
   call void undef(i64 addrspace(1)* %reloc)
   ret void
 ; CHECK-LABEL: test2
 ; CHECK-LABEL: equal
 ; CHECK: statepoint
 ; CHECK-NEXT: %reloc = call 
 ; CHECK-NEXT: call
 ; CHECK-NEXT: ret voi
 }
+
+; Basic test for invoke statepoints
+define i8 addrspace(1)* @test3(i8 addrspace(1)* %obj, i8 addrspace(1)* %obj1) gc "statepoint-example" {
+; CHECK-LABEL: test3
+entry:
+  ; CHECK-LABEL: entry
+  ; CHECK: statepoint
+  %0 = invoke i32 (void ()*, i32, i32, ...)* @llvm.experimental.gc.statepoint.p0f_isVoidf(void ()* undef, i32 0, i32 0, i32 5, i32 0, i32 -1, i32 0, i32 0, i32 0, i8 addrspace(1)* %obj, i8 addrspace(1)* %obj1)
+          to label %normal_dest unwind label %exceptional_return
+
+normal_dest:
+  ; CHECK-LABEL: normal_dest:
+  ; CHECK: gc.relocate
+  ; CHECK: gc.relocate
+  ; CHECK: ret
+  %obj.relocated = call coldcc i8 addrspace(1)* @llvm.experimental.gc.relocate.p1i8(i32 %0, i32 9, i32 9)
+  %obj1.relocated = call coldcc i8 addrspace(1)* @llvm.experimental.gc.relocate.p1i8(i32 %0, i32 10, i32 10)
+  ret i8 addrspace(1)* %obj.relocated
+
+exceptional_return:
+  ; CHECK-LABEL: exceptional_return
+  ; CHECK: gc.relocate
+  ; CHECK: gc.relocate
+  %landing_pad = landingpad { i8*, i32 } personality i32 ()* @"personality_function"
+          cleanup
+  %relocate_token = extractvalue { i8*, i32 } %landing_pad, 1
+  %obj.relocated1 = call coldcc i8 addrspace(1)* @llvm.experimental.gc.relocate.p1i8(i32 %relocate_token, i32 9, i32 9)
+  %obj1.relocated1 = call coldcc i8 addrspace(1)* @llvm.experimental.gc.relocate.p1i8(i32 %relocate_token, i32 10, i32 10)
+  ret i8 addrspace(1)* %obj1.relocated1
+}