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 ↗

(On Diff #19202)

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.

2366 ↗

(On Diff #19202)

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

2767 ↗

(On Diff #19202)

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)

2798 ↗

(On Diff #19202)

Please don't use statepoint here.

test/Verifier/statepoint.ll

55 ↗

(On Diff #19202)

add: gc "statepoint-example"

69 ↗

(On Diff #19202)

Does this extra branch add anything to this test?

[reames]

lib/IR/Verifier.cpp

2772 ↗

(On Diff #19372)

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

2787 ↗

(On Diff #19372)

I think this is checked elsewhere.

2798 ↗

(On Diff #19372)

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

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

2806 ↗

(On Diff #19372)

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

2787 ↗

(On Diff #19372)

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

2798 ↗

(On Diff #19202)

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.

llvm/trunk/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 (ExtractValueInst *ExtractValue =
+          dyn_cast<ExtractValueInst>(CI.getArgOperand(0))) {
+      Assert1(isa<LandingPadInst>(ExtractValue->getAggregateOperand()),
+              "gc relocate on unwind path incorrectly linked to the statepoint",
+              &CI);
+
+      const BasicBlock *invokeBB =
+        ExtractValue->getParent()->getUniquePredecessor();
+
+      // Landingpad relocates should have only one predecessor with invoke
+      // statepoint terminator
+      Assert1(invokeBB,
+              "safepoints should have unique landingpads",
+              ExtractValue->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
+      auto Token = CI.getArgOperand(0);
+      Assert2(isa<Instruction>(Token) && isStatepoint(cast<Instruction>(Token)),
+              "gc relocate is incorrectly tied to the statepoint",
+              &CI, Token);
+    }
+
+    // Verify rest of the relocate arguments
+
+    GCRelocateOperands ops(&CI);
+    ImmutableCallSite StatepointCS(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),

llvm/trunk/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
+}
+