[PATCH] Verification of invoke statepoints

[Philip Reames]

Comments inline.


REPOSITORY
  rL LLVM

================
Comment at: lib/IR/Verifier.cpp:1862
@@ -1861,1 +1861,3 @@
 
+  if (Function *F = II.getCalledFunction())
+    if (F->getIntrinsicID() == Intrinsic::experimental_gc_statepoint)
----------------
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.

================
Comment at: lib/IR/Verifier.cpp:2366
@@ -2362,1 +2365,3 @@
+              F->getIntrinsicID() == Intrinsic::experimental_patchpoint_i64 ||
+              isStatepoint(&I),
               "Cannot invoke an intrinsinc other than"
----------------
Given the structure of this clause, I'd prefer to see this as a test against the intrinsic id directly.  

================
Comment at: lib/IR/Verifier.cpp:2767
@@ +2766,3 @@
+    // 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,
----------------
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)

================
Comment at: lib/IR/Verifier.cpp:2798
@@ +2797,3 @@
+        return ImmutableCallSite(ops.statepoint());
+      return ImmutableCallSite(CI.getArgOperand(0));
+    }();
----------------
Please don't use statepoint here.

================
Comment at: test/Verifier/statepoint.ll:55
@@ +54,3 @@
+; Basic test for invoke statepoints
+define i8 addrspace(1)* @test3(i8 addrspace(1)* %obj, i8 addrspace(1)* %obj1) {
+; CHECK-LABEL: test3
----------------
add: gc "statepoint-example"



================
Comment at: test/Verifier/statepoint.ll:69
@@ +68,3 @@
+  %obj1.relocated = call coldcc i8 addrspace(1)* @llvm.experimental.gc.relocate.p1i8(i32 %0, i32 10, i32 10)
+  br label %normal_return
+
----------------
Does this extra branch add anything to this test?

http://reviews.llvm.org/D7366

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/