Reverse order in which base and derived pointers are lowered in the statepoint StackMap section

[igor-laevsky]

Summary

According to the documentation in StackMap section for the statepoint we should have:

The first Location in each pair describes the base pointer for the object. The second is the derived pointer actually being relocated.

But actually in the code we emit them in reverse order - derived pointer first, base pointer second.

This change fixes this problem and updates test cases to check ordering.

[reames]

I generally approve of the intent of the change. Comments inline.

p.s. Once we LGTM this, please hold off on submission until at least Monday of next week. I want interested parties to have a chance to see this.

lib/CodeGen/SelectionDAG/StatepointLowering.cpp

480–481

I would prefer to see this loop structured analogously to the other one you changed. As this change illustrates, the modulo trick here is unnecessarily complex and subject to mistakes.

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

11–12

This is an invalid test. An argument can not be a derived pointer. This is currently utterly unsupported.

You should create ptr_derived by bitcasting or gepping from the existing argument.

[igor-laevsky]

Restructured spill slot reserve loop and fixed test.

[igor-laevsky]

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

11–12

I fixed this test, but I am still not clear what do you mean by unsupported? It is true that in code generated with RewriteStatepointForGC pass we can not have such derived pointers. But from the lowering standpoint it does not make any difference how derived pointer were made. Also from the quick glance at the documentation I have not found that such cases are not supported. What am I missing?

[reames]

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

11–12

Hm, you're completely right here. I was confusing what RewriteStatepointsForGC knows how to handle with what the lowering code is required to handle. There's no reason the lowering code shouldn't handle this case correctly.

Can you add another test (copying what you had originally is fine) with a comment that explicitly says that? If I or someone else try to implement something under my broken assumption above, it would be good to have a test case to catch that.

[reames]

LGTM

[igor-laevsky]

Committed revision 237126.

lib/CodeGen/SelectionDAG/StatepointLowering.cpp

#endif
   // doesn't change semantics at all.  It is important for performance that we
   // reserve slots for both deopt and gc values before lowering either.
   for (auto I = StatepointSite.vm_state_begin() + 1,
             E = StatepointSite.vm_state_end();
        I != E; ++I) {
     Value *V = *I;
     SDValue Incoming = Builder.getValue(V);
     reservePreviousStackSlotForValue(Incoming, Builder);
   }
-  for (unsigned i = 0; i < Bases.size() * 2; ++i) {
-    // Even elements will contain base, odd elements - derived ptr
-    const Value *V = i % 2 ? Bases[i / 2] : Ptrs[i / 2];
-    SDValue Incoming = Builder.getValue(V);
-    reservePreviousStackSlotForValue(Incoming, Builder);
+  for (unsigned i = 0; i < Bases.size(); ++i) {
+    const Value *Base = Bases[i];
+    reservePreviousStackSlotForValue(Builder.getValue(Base), Builder);
+
+    const Value *Ptr = Ptrs[i];
+    reservePreviousStackSlotForValue(Builder.getValue(Ptr), Builder);
   }
 
   // First, prefix the list with the number of unique values to be
   // lowered.  Note that this is the number of *Values* not the
   // number of SDValues required to lower them.
   const int NumVMSArgs = StatepointSite.numTotalVMSArgs();
   Ops.push_back( Builder.DAG.getTargetConstant(StackMaps::ConstantOp,
                                                Builder.getCurSDLoc(),
for (auto I = StatepointSite.vm_state_begin() + 1,
     lowerIncomingStatepointValue(Incoming, Ops, Builder);
   }
 
   // Finally, go ahead and lower all the gc arguments.  There's no prefixed
   // length for this one.  After lowering, we'll have the base and pointer
   // arrays interwoven with each (lowered) base pointer immediately followed by
   // it's (lowered) derived pointer.  i.e
   // (base[0], ptr[0], base[1], ptr[1], ...)
-  for (unsigned i = 0; i < Bases.size() * 2; ++i) {
-    // Even elements will contain base, odd elements - derived ptr
-    const Value *V = i % 2 ? Bases[i / 2] : Ptrs[i / 2];
-    SDValue Incoming = Builder.getValue(V);
-    lowerIncomingStatepointValue(Incoming, Ops, Builder);
+  for (unsigned i = 0; i < Bases.size(); ++i) {
+    const Value *Base = Bases[i];
+    lowerIncomingStatepointValue(Builder.getValue(Base), Ops, Builder);
+
+    const Value *Ptr = Ptrs[i];
+    lowerIncomingStatepointValue(Builder.getValue(Ptr), Ops, Builder);
   }
 
   // If there are any explicit spill slots passed to the statepoint, record
   // them, but otherwise do not do anything special.  These are user provided
   // allocas and give control over placement to the consumer.  In this case,
   // it is the contents of the slot which may get updated, not the pointer to
   // the alloca
   for (Value *V : StatepointSite.gc_args()) {

test/CodeGen/X86/statepoint-stack-usage.ll

 ; RUN: llc < %s | FileCheck %s
 
 target datalayout = "e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-pc-linux-gnu"
 
 ; This test is checking to make sure that we reuse the same stack slots
 ; for GC values spilled over two different call sites.  Since the order
 ; of GC arguments differ, niave lowering code would insert loads and 
 ; stores to rearrange items on the stack.  We need to make sure (for
 ; performance) that this doesn't happen.
 define i32 @back_to_back_calls(i32 addrspace(1)* %a, i32 addrspace(1)* %b, i32 addrspace(1)* %c) #1 gc "statepoint-example" {
 ; CHECK-LABEL: back_to_back_calls
 ; The exact stores don't matter, but there need to be three stack slots created
-; CHECK: movq %rdx, 16(%rsp)
-; CHECK: movq %rdi, 8(%rsp)
+; CHECK: movq %rdi, 16(%rsp)
+; CHECK: movq %rdx, 8(%rsp)
 ; CHECK: movq %rsi, (%rsp)
   %safepoint_token = tail call 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, i32 addrspace(1)* %a, i32 addrspace(1)* %b, i32 addrspace(1)* %c)
   %a1 = tail call coldcc i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(i32 %safepoint_token, i32 9, i32 9)
   %b1 = tail call coldcc i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(i32 %safepoint_token, i32 9, i32 10)
   %c1 = tail call coldcc i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(i32 %safepoint_token, i32 9, i32 11)
 ; CHECK: callq
 ; This is the key check.  There should NOT be any memory moves here
 ; CHECK-NOT: movq
   %safepoint_token2 = tail call 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, i32 addrspace(1)* %c1, i32 addrspace(1)* %b1, i32 addrspace(1)* %a1)
   %a2 = tail call coldcc i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(i32 %safepoint_token2, i32 9, i32 11)
   %b2 = tail call coldcc i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(i32 %safepoint_token2, i32 9, i32 10)
   %c2 = tail call coldcc i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(i32 %safepoint_token2, i32 9, i32 9)
 ; CHECK: callq
   ret i32 1
 }
 
 ; This test simply checks that minor changes in vm state don't prevent slots
 ; being reused for gc values.  
 define i32 @reserve_first(i32 addrspace(1)* %a, i32 addrspace(1)* %b, i32 addrspace(1)* %c) #1 gc "statepoint-example" {
 ; CHECK-LABEL: reserve_first
 ; The exact stores don't matter, but there need to be three stack slots created
-; CHECK: movq %rdx, 16(%rsp)
-; CHECK: movq %rdi, 8(%rsp)
+; CHECK: movq %rdi, 16(%rsp)
+; CHECK: movq %rdx, 8(%rsp)
 ; CHECK: movq %rsi, (%rsp)
   %safepoint_token = tail call 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, i32 addrspace(1)* %a, i32 addrspace(1)* %b, i32 addrspace(1)* %c)
   %a1 = tail call coldcc i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(i32 %safepoint_token, i32 9, i32 9)
   %b1 = tail call coldcc i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(i32 %safepoint_token, i32 9, i32 10)
   %c1 = tail call coldcc i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(i32 %safepoint_token, i32 9, i32 11)
 ; CHECK: callq
 ; This is the key check.  There should NOT be any memory moves here
 ; CHECK-NOT: movq

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

 ; RUN: llc < %s | FileCheck %s
 ; This test is a sanity check to ensure statepoints are generating StackMap
 ; sections correctly.  This is not intended to be a rigorous test of the 
 ; StackMap format (see the stackmap tests for that).
 
 target datalayout = "e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-pc-linux-gnu"
 
 declare zeroext i1 @return_i1()
 
-define i1 @test(i32 addrspace(1)* %ptr) gc "statepoint-example" {
+define i1 @test(i32 addrspace(1)* %ptr_base, i32 %arg)
+  gc "statepoint-example" {
 ; CHECK-LABEL: test
-; Do we see one spill for the local value and the store to the
+; Do we see two spills for the local values and the store to the
 ; alloca?
-; CHECK: subq $24, %rsp
-; CHECK: movq $0, 8(%rsp)
-; CHECK: movq %rdi, (%rsp)
+; CHECK: subq $40, %rsp
+; CHECK: movq $0,   24(%rsp)
+; CHECK: movq %rdi, 16(%rsp)
+; CHECK: movq %rax, 8(%rsp)
 ; CHECK: callq return_i1
-; CHECK: addq $24, %rsp
+; CHECK: addq $40, %rsp
 ; CHECK: retq
 entry:
   %metadata1 = alloca i32 addrspace(1)*, i32 2, align 8
   store i32 addrspace(1)* null, i32 addrspace(1)** %metadata1
-  %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)
+  %ptr_derived = getelementptr i32, i32 addrspace(1)* %ptr_base, i32 %arg
+  %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_base, i32 addrspace(1)* null, i32 addrspace(1)* %ptr_base, i32 addrspace(1)* %ptr_derived, i32 addrspace(1)* null)
   %call1 = call zeroext i1 @llvm.experimental.gc.result.i1(i32 %safepoint_token)
   %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)
+  %b = call i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(i32 %safepoint_token, i32 6, i32 7)
+  %c = call i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(i32 %safepoint_token, i32 8, i32 8)
 ; 
   ret i1 %call1
 }
 
 declare i32 @llvm.experimental.gc.statepoint.p0f_i1f(i1 ()*, i32, i32, ...)
 declare i1 @llvm.experimental.gc.result.i1(i32)
 declare i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(i32, i32, i32) #3
 
 
 ; CHECK-LABEL: .section .llvm_stackmaps
 ; CHECK-NEXT:  __LLVM_StackMaps:
 ; Header
 ; CHECK-NEXT:   .byte 1
 ; CHECK-NEXT:   .byte 0
 ; CHECK-NEXT:   .short 0
 ; Num Functions
 ; CHECK-NEXT:   .long 1
 ; Num LargeConstants
 ; CHECK-NEXT:   .long 0
 ; Num Callsites
 ; CHECK-NEXT:   .long 1
 
 ; Functions and stack size
 ; CHECK-NEXT:   .quad test
-; CHECK-NEXT:   .quad 24
+; CHECK-NEXT:   .quad 40
 
 ; Large Constants
 ; Statepoint ID only
 ; CHECK: .quad  2882400000
 
 ; Callsites
 ; Constant arguments
 ; CHECK: .long  .Ltmp1-test
 ; CHECK: .short 0
-; CHECK: .short 8
+; CHECK: .short 10
 ; SmallConstant (0)
 ; CHECK: .byte  4
 ; CHECK: .byte  8
 ; CHECK: .short 0
 ; CHECK: .long  0
 ; SmallConstant (2)
 ; CHECK: .byte  4
 ; CHECK: .byte  8
 ; CHECK: .short 0
 ; CHECK: .long  2
 ; Direct Spill Slot [RSP+0]
 ; CHECK: .byte  2
 ; CHECK: .byte  8
 ; CHECK: .short 7
-; CHECK: .long  0
+; CHECK: .long  16
 ; SmallConstant  (0)
 ; CHECK: .byte  4
 ; CHECK: .byte  8
 ; CHECK: .short 0
 ; CHECK: .long  0
 ; SmallConstant  (0)
 ; CHECK: .byte  4
 ; CHECK: .byte  8
 ; CHECK: .short 0
 ; CHECK: .long  0
 ; SmallConstant  (0)
 ; CHECK: .byte  4
 ; CHECK: .byte  8
 ; CHECK: .short 0
 ; CHECK: .long  0
-; Direct Spill Slot [RSP+0]
+; Direct Spill Slot [RSP+16]
 ; CHECK: .byte  2
 ; CHECK: .byte  8
 ; CHECK: .short 7
-; CHECK: .long  0
-; Direct Spill Slot [RSP+0]
+; CHECK: .long  16
+; Direct Spill Slot [RSP+8]
 ; CHECK: .byte  2
 ; CHECK: .byte  8
 ; CHECK: .short 7
-; CHECK: .long  0
+; CHECK: .long  8
+; Direct Spill Slot [RSP+16]
+; CHECK: .byte  2
+; CHECK: .byte  8
+; CHECK: .short 7
+; CHECK: .long  16
+; Direct Spill Slot [RSP+16]
+; CHECK: .byte  2
+; CHECK: .byte  8
+; CHECK: .short 7
+; CHECK: .long  16
 
 ; No Padding or LiveOuts
 ; CHECK: .short 0
 ; CHECK: .short 0
 ; CHECK: .align 8