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.

2366

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

2767

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

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

2811

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

2826

I think this is checked elsewhere.

2837

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

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

2845

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

2798

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.

2826

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())
+    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 ||
+              isStatepoint(&I),
               "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: {
+    Assert1(CI.getNumArgOperands() == 3, "wrong number of arguments", &CI);
+
+    // We should be tied to either extractvalue from landingpad or statepoint
+    bool isLandingpad = isa<ExtractValueInst>(CI.getArgOperand(0));
+    Assert2(isStatepoint(cast<Instruction>(CI.getArgOperand(0))) || isLandingpad,
+            "token must be from a statepoint or landingpad", &CI, CI.getArgOperand(0));
+
+    if (isLandingpad) {
+      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 terminator invoke statepoint
+      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);
+    }
+
     // Are we tied to a statepoint properly?
-    CallSite StatepointCS(CI.getArgOperand(0));
+    GCRelocateOperands ops(&CI);
+    ImmutableCallSite StatepointCS = [&]() {
+      if (isLandingpad)
+        return ImmutableCallSite(ops.statepoint());
+      return ImmutableCallSite(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));
 
     // 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),
             "gc.relocate operand #3 must be integer offset", &CI);
 
     const int BaseIndex = cast<ConstantInt>(Base)->getZExtValue();
     const int DerivedIndex = cast<ConstantInt>(Derived)->getZExtValue();
     // Check the bounds
     Assert1(0 <= BaseIndex &&
             BaseIndex < (int)StatepointCS.arg_size(),
             "gc.relocate: statepoint base index out of bounds", &CI);
     Assert1(0 <= DerivedIndex &&
             DerivedIndex < (int)StatepointCS.arg_size(),
             "gc.relocate: statepoint derived index out of bounds", &CI);
 
     // Check that BaseIndex and DerivedIndex fall within the 'gc parameters'
     // section of the statepoint's argument
     const int NumCallArgs =
       cast<ConstantInt>(StatepointCS.getArgument(1))->getZExtValue();
     const int NumDeoptArgs =
       cast<ConstantInt>(StatepointCS.getArgument(NumCallArgs + 3))->getZExtValue();
     const int GCParamArgsStart = NumCallArgs + NumDeoptArgs + 4;
     const int GCParamArgsEnd = StatepointCS.arg_size();
     Assert1(GCParamArgsStart <= BaseIndex &&
             BaseIndex < GCParamArgsEnd,
             "gc.relocate: statepoint base index doesn't fall within the "
       "'gc parameters' section of the statepoint call", &CI);
     Assert1(GCParamArgsStart <= DerivedIndex &&
             DerivedIndex < GCParamArgsEnd,
             "gc.relocate: statepoint derived index doesn't fall within the "
       "'gc parameters' section of the statepoint call", &CI);
 
 
     // Assert that the result type matches the type of the relocated pointer
     GCRelocateOperands Operands(&CI);
     Assert1(Operands.derivedPtr()->getType() == CI.getType(),
             "gc.relocate: relocating a pointer shouldn't change its type",
             &CI);
     break;
   }
   };
 }

test/Verifier/statepoint.ll

equal:
   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) {
+; 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 %invoke_safepoint_normal_dest unwind label %exceptional_return
+
+invoke_safepoint_normal_dest:
+  ; CHECK-LABEL: invoke_safepoint_normal_dest
+  ; CHECK: gc.relocate
+  ; CHECK: gc.relocate
+  %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)
+  br label %normal_return
+
+normal_return:
+  ; CHECK-LABEL: normal_return
+  ; CHECK: ret
+  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
+}