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

[artagnon]

Summary

This patch started out as an exploration of gc.relocate, and an attempt
to write a simple test in call-lowering. I then noticed that the
arguments of gc.relocate were not checked fully, so I went in and fixed
a few things. Finally, the most important outcome of this patch is that
my new error handling code caught a bug in a callsite in
stackmap-format.

[sanjoy]

Some small nits inline. I'll defer to Philip for an LGTM.

lib/IR/Verifier.cpp

2695 ↗

(On Diff #17755)

Should be nullptr

2734 ↗

(On Diff #17755)

Might be cleaner to store the result of getZExtValue() in an unsigned instead.

test/CodeGen/X86/statepoint-stackmap-format.ll

30 ↗

(On Diff #17755)

You had to make these changes because the original indices are out of range, right?

[reames]

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

lib/IR/Verifier.cpp

2695 ↗

(On Diff #17755)

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.)

2734 ↗

(On Diff #17755)

While Sanjoy's right, you'd don't need to fix my pre-existing style problems in your change. :)

test/CodeGen/X86/statepoint-call-lowering.ll

65 ↗

(On Diff #17755)

"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?

test/CodeGen/X86/statepoint-stackmap-format.ll

30 ↗

(On Diff #17755)

I'll just say "oops!". Thanks for catching this.

[artagnon]

lib/IR/Verifier.cpp

2734 ↗

(On Diff #17755)

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?

test/CodeGen/X86/statepoint-stackmap-format.ll

30 ↗

(On Diff #17755)

Technically, the original indices didn't fall within the 'gc parameters' section of the arguments: argument #3 reads i32 2, which means that '4' and '5' refer to the two following deopt parameters.

[artagnon]

lib/IR/Verifier.cpp

2695 ↗

(On Diff #17755)

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.

test/CodeGen/X86/statepoint-call-lowering.ll

65 ↗

(On Diff #17755)

Yep, that's exactly what I intended: to show that it isn't lowered. Should I put a CHECK-NOT or something?

[artagnon]

Fix nits caught by {sanjoy,reames} review.

[reames]

lib/IR/Verifier.cpp

2695 ↗

(On Diff #17755)

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.

2734 ↗

(On Diff #17755)

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.

test/CodeGen/X86/statepoint-call-lowering.ll

65 ↗

(On Diff #17755)

Use check-next to make sure there's no other instructions and add a comment stating that. It's not at all obvious.

[artagnon]

1. Revert const int -> unsigned changes, since reames would like it in a different commit.

2. Use CHECK-NEXT as reames suggests, in the test. Clarify what we're testing.

3. Leave the getInstruction() ternary check intact. We might factor it out in a separate change.

[reames]

LGTM w/minor comment tweak. Do you want me to submit on your behalf?

test/CodeGen/X86/statepoint-call-lowering.ll

65 ↗

(On Diff #17765)

You're missing something key here. A suggestion:
"This is checking that an *unused* relocate has no code generation impact."

[artagnon]

Sure, do submit with the comment tweak.

I'm yet to figure out how relocate should actually be used: I didn't quite get what happens when base index = derived index.

[artagnon]

(After watching the LLVMDev talk): Did you mean that the result of the relocate is unused, and hence no code is generated? If I did %a = gc.relocate(...), and then used %a in another gc.statepoint call, then a mov will be generated like in statepoint-stack-usage.ll? The mov is generated because it's assuming that the gc is relocating the base, and then returning a new derived with some offset (= the original derived - base) for use with the second gc.statepoint?

[artagnon]

Ping.

Reames: would you like me to do anything else?

[reames]

In D6824#105192, @artagnon wrote:

I'm yet to figure out how relocate should actually be used: I didn't quite get what happens when base index = derived index.

This means that the pointer itself is a base pointer. This is extremely common and well supported.

[reames]

In D6824#105194, @artagnon wrote:

(After watching the LLVMDev talk): Did you mean that the result of the relocate is unused, and hence no code is generated? If I did %a = gc.relocate(...), and then used %a in another gc.statepoint call, then a mov will be generated like in statepoint-stack-usage.ll? The mov is generated because it's assuming that the gc is relocating the base, and then returning a new derived with some offset (= the original derived - base) for use with the second gc.statepoint?

I'm having a hard time parsing your question.

Let me lay out a few facts; I think that together these address what you're getting at.

1. Once inserted, any use (even a trivial one) will cause the pointer to be saved in the stackmap.
2. For a derived pointer to be relocated, it's base pointer must be as well. (If the derived pointer is a base pointer, this is trivial.)
3. The offset between a relocated derived pointer and it's relocated base must be the same as the offset between the original derived pointer and the original base.

W.r.t. this patch, is it ready for submission? Or does the clarification above change something? Once you consider it ready, let me know and I'll submit on your behalf.

[artagnon]

Update comment in call-lowering, and rebase onto latest master.

@reames: Thanks for the clarifying comments; I have a better
understanding now. It's ready to land now, I think.

llvm/trunk/lib/IR/Verifier.cpp

case Intrinsic::experimental_gc_statepoint: {
     // about.  See example statepoint.ll in the verifier subdirectory
     break;
   }
   case Intrinsic::experimental_gc_result_int:
   case Intrinsic::experimental_gc_result_float:
   case Intrinsic::experimental_gc_result_ptr: {
     // Are we tied to a statepoint properly?
     CallSite StatepointCS(CI.getArgOperand(0));
-    const Function *StatepointFn = StatepointCS.getCalledFunction();
+    const Function *StatepointFn =
+      StatepointCS.getInstruction() ? StatepointCS.getCalledFunction() : nullptr;
     Assert2(StatepointFn && StatepointFn->isDeclaration() &&
             StatepointFn->getIntrinsicID() == Intrinsic::experimental_gc_statepoint,
-            "token must be from a statepoint", &CI, CI.getArgOperand(0));
+      "gc.result operand #1 must be from a statepoint",
+      &CI, CI.getArgOperand(0));
 
     // Assert that result type matches wrapped callee.
     const Value *Target = StatepointCS.getArgument(0);
     const PointerType *PT = cast<PointerType>(Target->getType());
     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() : NULL;
+        StatepointCS.getInstruction() ? StatepointCS.getCalledFunction() : nullptr;
     Assert2(StatepointFn && StatepointFn->isDeclaration() &&
             StatepointFn->getIntrinsicID() == Intrinsic::experimental_gc_statepoint,
-            "token must be from a statepoint", &CI, CI.getArgOperand(0));
+            "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), "must be integer offset", &CI);
+    Assert1(isa<ConstantInt>(Base),
+            "gc.relocate operand #2 must be integer offset", &CI);
     
     Value* Derived = CI.getArgOperand(2);
-    Assert1( isa<ConstantInt>(Derived), "must be integer offset", &CI);
+    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(),
-            "index out of bounds", &CI);
+            "gc.relocate: statepoint base index out of bounds", &CI);
     Assert1(0 <= DerivedIndex &&
             DerivedIndex < (int)StatepointCS.arg_size(),
-            "index out of bounds", &CI);
+            "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 it's type",
+            "gc.relocate: relocating a pointer shouldn't change its type",
             &CI);
     break;
   }
   };
 }
 
 void DebugInfoVerifier::verifyDebugInfo() {
   if (!VerifyDebugInfo)

llvm/trunk/test/CodeGen/X86/statepoint-call-lowering.ll

 ; CHECK: popq %rax
 ; CHECK: retq
 entry:
   %safepoint_token = tail call i32 (float ()*, i32, i32, ...)* @llvm.experimental.gc.statepoint.p0f_f32f(float ()* @return_float, i32 0, i32 0, i32 0)
   %call1 = call float @llvm.experimental.gc.result.float.f32(i32 %safepoint_token)
   ret float %call1
 }
 
+define i1 @test_relocate(i32* %a) {
+; CHECK-LABEL: test_relocate
+; Check that an ununsed relocate has no code-generation impact
+; CHECK: pushq %rax
+; CHECK: callq return_i1
+; CHECK-NEXT: .Ltmp13:
+; CHECK-NEXT: popq %rdx
+; CHECK-NEXT: retq
+entry:
+  %safepoint_token = tail call i32 (i1 ()*, i32, i32, ...)* @llvm.experimental.gc.statepoint.p0f_i1f(i1 ()* @return_i1, i32 0, i32 0, i32 0, i32* %a)
+  %call1 = call i32* @llvm.experimental.gc.relocate.p0i32(i32 %safepoint_token, i32 4, i32 4)
+  %call2 = call zeroext i1 @llvm.experimental.gc.result.int.i1(i32 %safepoint_token)
+  ret i1 %call2
+}
+
 declare i32 @llvm.experimental.gc.statepoint.p0f_i1f(i1 ()*, i32, i32, ...)
 declare i1 @llvm.experimental.gc.result.int.i1(i32)
 
 declare i32 @llvm.experimental.gc.statepoint.p0f_i32f(i32 ()*, i32, i32, ...)
 declare i32 @llvm.experimental.gc.result.int.i32(i32)
 
 declare i32 @llvm.experimental.gc.statepoint.p0f_p0i32f(i32* ()*, i32, i32, ...)
 declare i32* @llvm.experimental.gc.result.ptr.p0i32(i32)
 
 declare i32 @llvm.experimental.gc.statepoint.p0f_f32f(float ()*, i32, i32, ...)
 declare float @llvm.experimental.gc.result.float.f32(i32)
 
+declare i32* @llvm.experimental.gc.relocate.p0i32(i32, i32, i32)

llvm/trunk/test/CodeGen/X86/statepoint-stackmap-format.ll

 ; CHECK: movq $0, 8(%rsp)
 ; CHECK: movq %rdi, (%rsp)
 ; CHECK: callq return_i1
 ; CHECK: addq $24, %rsp
 ; CHECK: retq
 entry:
   %metadata1 = alloca i32 addrspace(1)*, i32 2, align 8
   store i32 addrspace(1)* null, i32 addrspace(1)** %metadata1
-; NOTE: Currently NOT testing alloca lowering in the StackMap format.  Its 
-; known to be broken.  
-  %safepoint_token = tail call i32 (i1 ()*, i32, i32, ...)* @llvm.experimental.gc.statepoint.p0f_i1f(i1 ()* @return_i1, i32 0, i32 0, i32 2, i32 addrspace(1)* %ptr, i32 addrspace(1)* null)
+  %safepoint_token = tail call i32 (i1 ()*, i32, i32, ...)* @llvm.experimental.gc.statepoint.p0f_i1f(i1 ()* @return_i1, i32 0, i32 0, i32 2, i32 addrspace(1)* %ptr, i32 addrspace(1)* null, i32 addrspace(1)* %ptr, i32 addrspace(1)* null)
   %call1 = call zeroext i1 @llvm.experimental.gc.result.int.i1(i32 %safepoint_token)
-  %a = call i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(i32 %safepoint_token, i32 4, i32 4)
-  %b = call i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(i32 %safepoint_token, i32 5, i32 5)
+  %a = call i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(i32 %safepoint_token, i32 6, i32 6)
+  %b = call i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(i32 %safepoint_token, i32 7, i32 7)
 ; 
   ret i1 %call1
 }
 
 declare i32 @llvm.experimental.gc.statepoint.p0f_i1f(i1 ()*, i32, i32, ...)
 declare i1 @llvm.experimental.gc.result.int.i1(i32)
 declare i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(i32, i32, i32) #3