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

[Philip Reames]

================
Comment at: lib/IR/Verifier.cpp:2695
@@ -2695,1 +2694,3 @@
+    const Function *StatepointFn =
+      StatepointCS.getInstruction() ? StatepointCS.getCalledFunction() : NULL;
     Assert2(StatepointFn && StatepointFn->isDeclaration() &&
----------------
artagnon wrote:
> reames wrote:
> > 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.)
> No, I didn't make it actually fail; I just noticed this check in surrounding code and thought I should put it in here too. It's not about an early exit from `getInstruction()`; it's about reporting the correct frontend error, I think.
Either is fine.  You could be slightly more fine grained, but I don't see that doing so actually adds anything.

i.e. The difference between "this is not a statepoint token" and "this is not a statepoint token and not a call or inst" seems minor at best.

================
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);
----------------
artagnon wrote:
> reames wrote:
> > 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.  :)
> Actually, I copied that out from the gc_statepoint case code: would you like me to change the instances on lines 2624 and 2650 too, for consistency?
If you're going to change it once, please change it everywhere.  I would prefer you did that as a separate change, but not strongly so.

================
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
----------------
artagnon wrote:
> reames wrote:
> > "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?
> Yep, that's exactly what I intended: to show that it isn't lowered. Should I put a CHECK-NOT or something?
Use check-next to make sure there's no other instructions and add a comment stating that.  It's not at all obvious.

http://reviews.llvm.org/D6824

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