[PATCH] [GC] improve testing around gc.relocate and catch a bug

[Philip Reames]

LGTM w/minor comments addressed.  Let me know if you want me to submit.


================
Comment at: lib/IR/Verifier.cpp:2695
@@ -2695,1 +2694,3 @@
+    const Function *StatepointFn =
+      StatepointCS.getInstruction() ? StatepointCS.getCalledFunction() : NULL;
     Assert2(StatepointFn && StatepointFn->isDeclaration() &&
----------------
sanjoy wrote:
> Should be `nullptr`
Did you manage to find a way to make this fail without the change?  

I might structure this as an assert and early exit on the getInstruction check.  It would be clearer.  (This is entirely optional.)

================
Comment at: lib/IR/Verifier.cpp:2734
@@ -2728,2 +2733,3 @@
     Assert1(0 <= BaseIndex &&
             BaseIndex < (int)StatepointCS.arg_size(),
+            "gc.relocate: statepoint base index out of bounds", &CI);
----------------
sanjoy wrote:
> Might be cleaner to store the result of `getZExtValue()` in an `unsigned` instead.
While Sanjoy's right, you'd don't need to fix my pre-existing style problems in your change.  :)

================
Comment at: test/CodeGen/X86/statepoint-call-lowering.ll:65
@@ +64,3 @@
+; CHECK-LABEL: test_relocate
+; This is just checking that a no-op relocate
+; CHECK: pushq %rax
----------------
"that a no-op relocate" what?

p.s. Given the relocated value isn't used, this is testing less than it might seem.  In particular, the 'relocation' is not actual lowered and exists only in the IR.  Is that what you intended?

================
Comment at: test/CodeGen/X86/statepoint-stackmap-format.ll:30
@@ -30,2 +29,3 @@
+  %b = call i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(i32 %safepoint_token, i32 7, i32 7)
 ; 
   ret i1 %call1
----------------
sanjoy wrote:
> You had to make these changes because the original indices are out of range, right?
I'll just say "oops!".  Thanks for catching this.

http://reviews.llvm.org/D6824

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