[Statepoints] Relocating vectors of pointers

[reames]

Summary

Currently, we try to split vectors of pointers back into their component pointer elements during rewrite-statepoints-for-gc. This is less than ideal since presumably the vectorizer chose to vectorize for a reason. :) It's also been a source of bugs - in particular, the relocation logic as currently implemented was recently discovered to be wrong.

The alternate approach is to allow gc.relocates of vector-of-pointer type and update the backend to handle them. That's what this patch tries to do. This won't actually enable vector-of-pointers in practice - there are some RS4GC changes needed - but the lowering is standalone and testable so it makes sense to separate.

Note that there are some known cases around vector constants which this patch does not handle. Once this is in, I'll send another patch with test cases. I decided to separate them since it would let me make progress on other parts - RS4GC, frontend changes - in parallel. The additional changes required don't look particularly complex.

[sanjoy]

As I've said in person, I'm worried about LLVM (SelectionDAG & later,
specifically) making assumptions on how spill slots can be used. For
instance, if there is an assumption that spill slots are strictly
frame local, then this transform becomes legal (gc_ref is a callee
saved register):

[%rsp + 48] = gc_ref
call _foo ## statepoint
gc_ref_relocated = [%rsp + 48]

>

[%rsp + 48] = gc_ref
gc_ref_relocated = [%rsp + 48]
call _foo ## statepoint

Since the call to _foo could not have possibly clobbered a frame local
location (since the frame is where you spill registers to *avoid* them
getting clobbered!).

In fact, I suspect this is unsound even today. I still can't see the
whole picture, but for instance, in MachineLICM.cpp you have:

static bool InstructionStoresToFI(const MachineInstr *MI, int FI) {
  for (MachineInstr::mmo_iterator o = MI->memoperands_begin(),
         oe = MI->memoperands_end(); o != oe; ++o) {
    if (!(*o)->isStore() || !(*o)->getPseudoValue())
      continue;
    if (const FixedStackPseudoSourceValue *Value =
        dyn_cast<FixedStackPseudoSourceValue>((*o)->getPseudoValue())) {
      if (Value->getFrameIndex() == FI)
        return true;
    }
  }
  return false;
}

and I don't see how a STATEPOINT node (as created in
StatepointLowering.cpp) gets a FixedStackPseudoSourceValue in its
memoperands vector (the loads and stores we generate around the
STATEPOINT node the right aliasing information).

[reames]

In D15632#313619, @sanjoy wrote:

As I've said in person, I'm worried about LLVM (SelectionDAG & later,
specifically) making assumptions on how spill slots can be used.

The general concern is a valid one. I think I've talked myself/you've talked me around to an alternate approach. If we add new flags to StackObject called 'isDeoptSpil' and 'isGCSpill' we can customize the behaviour much more obviously and safely. I'm going to restructure this patch in that direction unless someone speaks up to say that's a bad idea.

For
instance, if there is an assumption that spill slots are strictly
frame local, then this transform becomes legal (gc_ref is a callee
saved register):

[%rsp + 48] = gc_ref
call _foo ## statepoint
gc_ref_relocated = [%rsp + 48]

>

[%rsp + 48] = gc_ref
gc_ref_relocated = [%rsp + 48]
call _foo ## statepoint

Since the call to _foo could not have possibly clobbered a frame local
location (since the frame is where you spill registers to *avoid* them
getting clobbered!).

This is supposed to be handled by the volatile, load, and store flags we put on the memory operands in emitPatchPoint, but see below....

In fact, I suspect this is unsound even today. I still can't see the
whole picture, but for instance, in MachineLICM.cpp you have:

static bool InstructionStoresToFI(const MachineInstr *MI, int FI) {
  for (MachineInstr::mmo_iterator o = MI->memoperands_begin(),
         oe = MI->memoperands_end(); o != oe; ++o) {
    if (!(*o)->isStore() || !(*o)->getPseudoValue())
      continue;
    if (const FixedStackPseudoSourceValue *Value =
        dyn_cast<FixedStackPseudoSourceValue>((*o)->getPseudoValue())) {
      if (Value->getFrameIndex() == FI)
        return true;
    }
  }
  return false;
}

To the best of my knowledge, this code is incorrect. See http://reviews.llvm.org/D15730 with a proposed fix.

and I don't see how a STATEPOINT node (as created in
StatepointLowering.cpp) gets a FixedStackPseudoSourceValue in its
memoperands vector (the loads and stores we generate around the
STATEPOINT node the right aliasing information).

This is done in emitPatchPoint at around 1129 in CodeGen/TargetLoweringBase.cpp

[sanjoy]

In D15632#315852, @reames wrote:

This is done in emitPatchPoint at around 1129 in CodeGen/TargetLoweringBase.cpp

I was only looking at StatepointLowering.cpp -- there we create a
STATEPOINT node without any indication that it may touch the created
spill slots (as opposed to the store and load instructions we create).
However, if the invariant is that "no mem operands == touches the
whole heap", then this is fine.

[reames]

This patch needs to be rebased on http://reviews.llvm.org/D15759 "[Statepoints] Use Indirect operands for spill slots" and 256312: "[GC] Make GCStrategy::isGCManagedPointer a type predicate not a value", before further review. Note that the approach described in this review is different from the one I settled on in D15759.

[reames]

Rebased patch. Ready for review. Removing several reviewers who are likely not interested in the remaining changes since the backend bits were separated and already landed.

[reames]

ping!

[pgavlin]

LGTM.

llvm/trunk/include/llvm/IR/Intrinsics.td

 def int_experimental_gc_statepoint : Intrinsic<[llvm_token_ty],
                                [llvm_i64_ty, llvm_i32_ty,
                                 llvm_anyptr_ty, llvm_i32_ty,
                                 llvm_i32_ty, llvm_vararg_ty],
                                 [Throws]>;
 
 def int_experimental_gc_result   : Intrinsic<[llvm_any_ty], [llvm_token_ty],
                                              [IntrReadMem]>;
-def int_experimental_gc_relocate : Intrinsic<[llvm_anyptr_ty],
+def int_experimental_gc_relocate : Intrinsic<[llvm_any_ty],
                                 [llvm_token_ty, llvm_i32_ty, llvm_i32_ty],
                                 [IntrReadMem]>;
 
 //===-------------------------- Other Intrinsics --------------------------===//
 //
 def int_flt_rounds : Intrinsic<[llvm_i32_ty]>,
                      GCCBuiltin<"__builtin_flt_rounds">;
 def int_trap : Intrinsic<[], [], [IntrNoReturn]>,

llvm/trunk/lib/CodeGen/SelectionDAG/StatepointLowering.cpp

static void lowerStatepointMetaArgs(SmallVectorImpl<SDValue> &Ops,
   // be: deopt argument length, deopt arguments.., gc arguments...
 
   SmallVector<const Value *, 64> Bases, Ptrs, Relocations;
   getIncomingStatepointGCValues(Bases, Ptrs, Relocations, StatepointSite,
                                 Builder);
 
 #ifndef NDEBUG
   // Check that each of the gc pointer and bases we've gotten out of the
-  // safepoint is something the strategy thinks might be a pointer into the GC
-  // heap.  This is basically just here to help catch errors during statepoint
-  // insertion. TODO: This should actually be in the Verifier, but we can't get
-  // to the GCStrategy from there (yet).
+  // safepoint is something the strategy thinks might be a pointer (or vector
+  // of pointers) into the GC heap.  This is basically just here to help catch
+  // errors during statepoint insertion. TODO: This should actually be in the
+  // Verifier, but we can't get to the GCStrategy from there (yet).
   GCStrategy &S = Builder.GFI->getStrategy();
   for (const Value *V : Bases) {
-    auto Opt = S.isGCManagedPointer(V->getType());
+    auto Opt = S.isGCManagedPointer(V->getType()->getScalarType());
     if (Opt.hasValue()) {
       assert(Opt.getValue() &&
              "non gc managed base pointer found in statepoint");
     }
   }
   for (const Value *V : Ptrs) {
-    auto Opt = S.isGCManagedPointer(V->getType());
+    auto Opt = S.isGCManagedPointer(V->getType()->getScalarType());
     if (Opt.hasValue()) {
       assert(Opt.getValue() &&
              "non gc managed derived pointer found in statepoint");
     }
   }
   for (const Value *V : Relocations) {
-    auto Opt = S.isGCManagedPointer(V->getType());
+    auto Opt = S.isGCManagedPointer(V->getType()->getScalarType());
     if (Opt.hasValue()) {
       assert(Opt.getValue() && "non gc managed pointer relocated");
     }
   }
 #endif
 
   // Before we actually start lowering (and allocating spill slots for values),
   // reserve any stack slots which we judge to be profitable to reuse for a

llvm/trunk/lib/IR/Verifier.cpp

case Intrinsic::experimental_gc_result: {
     auto *TargetFuncType = cast<FunctionType>(PT->getElementType());
     Assert(CS.getType() == TargetFuncType->getReturnType(),
            "gc.result result type does not match wrapped callee", CS);
     break;
   }
   case Intrinsic::experimental_gc_relocate: {
     Assert(CS.getNumArgOperands() == 3, "wrong number of arguments", CS);
 
+    Assert(isa<PointerType>(CS.getType()->getScalarType()),
+           "gc.relocate must return a pointer or a vector of pointers", CS);
+
     // Check that this relocate is correctly tied to the statepoint
 
     // This is case for relocate on the unwinding path of an invoke statepoint
     if (LandingPadInst *LandingPad =
           dyn_cast<LandingPadInst>(CS.getArgOperand(0))) {
 
       const BasicBlock *InvokeBB =
           LandingPad->getParent()->getUniquePredecessor();
Assert(GCParamArgsStart <= BaseIndex && BaseIndex < GCParamArgsEnd,
            "gc.relocate: statepoint base index doesn't fall within the "
            "'gc parameters' section of the statepoint call",
            CS);
     Assert(GCParamArgsStart <= DerivedIndex && DerivedIndex < GCParamArgsEnd,
            "gc.relocate: statepoint derived index doesn't fall within the "
            "'gc parameters' section of the statepoint call",
            CS);
 
-    // Relocated value must be a pointer type, but gc_relocate does not need to return the
-    // same pointer type as the relocated pointer. It can be casted to the correct type later
-    // if it's desired. However, they must have the same address space.
+    // Relocated value must be either a pointer type or vector-of-pointer type,
+    // but gc_relocate does not need to return the same pointer type as the
+    // relocated pointer. It can be casted to the correct type later if it's
+    // desired. However, they must have the same address space and 'vectorness'
     GCRelocateInst &Relocate = cast<GCRelocateInst>(*CS.getInstruction());
-    Assert(Relocate.getDerivedPtr()->getType()->isPointerTy(),
+    Assert(Relocate.getDerivedPtr()->getType()->getScalarType()->isPointerTy(),
            "gc.relocate: relocated value must be a gc pointer", CS);
 
-    // gc_relocate return type must be a pointer type, and is verified earlier in
-    // VerifyIntrinsicType().
-    Assert(cast<PointerType>(CS.getType())->getAddressSpace() ==
-           cast<PointerType>(Relocate.getDerivedPtr()->getType())->getAddressSpace(),
+    auto ResultType = CS.getType();
+    auto DerivedType = Relocate.getDerivedPtr()->getType();
+    Assert(ResultType->isVectorTy() == DerivedType->isVectorTy(),
+           "gc.relocate: vector relocates to vector and pointer to pointer", CS);
+    Assert(ResultType->getPointerAddressSpace() ==
+           DerivedType->getPointerAddressSpace(),
            "gc.relocate: relocating a pointer shouldn't change its address space", CS);
     break;
   }
   case Intrinsic::eh_exceptioncode:
   case Intrinsic::eh_exceptionpointer: {
     Assert(isa<CatchPadInst>(CS.getArgOperand(0)),
            "eh.exceptionpointer argument must be a catchpad", CS);
     break;

llvm/trunk/test/CodeGen/X86/statepoint-vector.ll

+; RUN: llc -debug-only=stackmaps < %s | FileCheck %s
+
+; Can we lower a single vector?
+define <2 x i8 addrspace(1)*> @test(<2 x i8 addrspace(1)*> %obj) gc "statepoint-example" {
+entry:
+; CHECK-LABEL: @test
+; CHECK: subq $24, %rsp
+; CHECK: movaps %xmm0, (%rsp)
+; CHECK: callq  do_safepoint
+; CHECK: movaps (%rsp), %xmm0
+; CHECK: addq $24, %rsp
+  %safepoint_token = call token (i64, i32, void ()*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_isVoidf(i64 0, i32 0, void ()* @do_safepoint, i32 0, i32 0, i32 0, i32 0, <2 x i8 addrspace(1)*> %obj)
+  %obj.relocated = call coldcc <2 x i8 addrspace(1)*> @llvm.experimental.gc.relocate.v2p1i8(token %safepoint_token, i32 7, i32 7) ; (%obj, %obj)
+  ret <2 x i8 addrspace(1)*> %obj.relocated
+}
+
+; Can we lower the base, derived pairs if both are vectors?
+define <2 x i8 addrspace(1)*> @test2(<2 x i8 addrspace(1)*> %obj, i64 %offset) gc "statepoint-example" {
+entry:
+; CHECK-LABEL: @test2
+; CHECK: subq $40, %rsp
+; CHECK: movd %rdi, %xmm1
+; CHECK: pshufd $68, %xmm1, %xmm1       # xmm1 = xmm1[0,1,0,1]
+; CHECK: paddq  %xmm0, %xmm1
+; CHECK: movdqa %xmm0, 16(%rsp)
+; CHECK: movdqa %xmm1, (%rsp)
+; CHECK: callq  do_safepoint
+; CHECK: movaps (%rsp), %xmm0
+; CHECK: addq $40, %rsp
+  %derived = getelementptr i8, <2 x i8 addrspace(1)*> %obj, i64 %offset
+  %safepoint_token = call token (i64, i32, void ()*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_isVoidf(i64 0, i32 0, void ()* @do_safepoint, i32 0, i32 0, i32 0, i32 0, <2 x i8 addrspace(1)*> %obj, <2 x i8 addrspace(1)*> %derived)
+  %derived.relocated = call coldcc <2 x i8 addrspace(1)*> @llvm.experimental.gc.relocate.v2p1i8(token %safepoint_token, i32 7, i32 8) ; (%obj, %derived)
+  ret <2 x i8 addrspace(1)*> %derived.relocated
+}
+
+; Originally, this was just a variant of @test2 above, but it ends up 
+; covering a bunch of interesting missed optimizations.  Specifically:
+; - We waste a stack slot for a value that a backend transform pass
+;   CSEd to another spilled one.
+; - We don't remove the testb even though it serves no purpose
+; - We could in principal reuse the argument memory (%rsi) and do away
+;   with stack slots entirely.
+define <2 x i64 addrspace(1)*> @test3(i1 %cnd, <2 x i64 addrspace(1)*>* %ptr) gc "statepoint-example" {
+entry:
+; CHECK-LABEL: @test3
+; CHECK: subq $40, %rsp
+; CHECK: testb  $1, %dil
+; CHECK: movaps (%rsi), %xmm0
+; CHECK: movaps %xmm0, 16(%rsp)
+; CHECK: movaps %xmm0, (%rsp)
+; CHECK: callq  do_safepoint
+; CHECK: movaps (%rsp), %xmm0
+; CHECK: addq $40, %rsp
+  br i1 %cnd, label %taken, label %untaken
+
+taken:                                            ; preds = %entry
+  %obja = load <2 x i64 addrspace(1)*>, <2 x i64 addrspace(1)*>* %ptr
+  br label %merge
+
+untaken:                                          ; preds = %entry
+  %objb = load <2 x i64 addrspace(1)*>, <2 x i64 addrspace(1)*>* %ptr
+  br label %merge
+
+merge:                                            ; preds = %untaken, %taken
+  %obj.base = phi <2 x i64 addrspace(1)*> [ %obja, %taken ], [ %objb, %untaken ]
+  %obj = phi <2 x i64 addrspace(1)*> [ %obja, %taken ], [ %objb, %untaken ]
+  %safepoint_token = call token (i64, i32, void ()*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_isVoidf(i64 0, i32 0, void ()* @do_safepoint, i32 0, i32 0, i32 0, i32 0, <2 x i64 addrspace(1)*> %obj, <2 x i64 addrspace(1)*> %obj.base)
+  %obj.relocated = call coldcc <2 x i8 addrspace(1)*> @llvm.experimental.gc.relocate.v2p1i8(token %safepoint_token, i32 8, i32 7) ; (%obj.base, %obj)
+  %obj.relocated.casted = bitcast <2 x i8 addrspace(1)*> %obj.relocated to <2 x i64 addrspace(1)*>
+  %obj.base.relocated = call coldcc <2 x i8 addrspace(1)*> @llvm.experimental.gc.relocate.v2p1i8(token %safepoint_token, i32 8, i32 8) ; (%obj.base, %obj.base)
+  %obj.base.relocated.casted = bitcast <2 x i8 addrspace(1)*> %obj.base.relocated to <2 x i64 addrspace(1)*>
+  ret <2 x i64 addrspace(1)*> %obj.relocated.casted
+}
+
+; CHECK: __LLVM_StackMaps:
+
+; CHECK: .Ltmp1-test
+; Check for the two spill slots
+; Stack Maps:     Loc 3: Indirect 7+0 [encoding: .byte 3, .byte 16, .short 7, .int 0]
+; Stack Maps:     Loc 4: Indirect 7+0 [encoding: .byte 3, .byte 16, .short 7, .int 0]
+; CHECK: .byte  3
+; CHECK: .byte  16
+; CHECK: .short 7
+; CHECK: .long  0
+; CHECK: .byte  3
+; CHECK: .byte  16
+; CHECK: .short 7
+; CHECK: .long  0
+
+; CHECK: .Ltmp3-test2
+; Check for the two spill slots
+; Stack Maps:     Loc 3: Indirect 7+16  [encoding: .byte 3, .byte 16, .short 7, .int 16]
+; Stack Maps:     Loc 4: Indirect 7+0 [encoding: .byte 3, .byte 16, .short 7, .int 0]
+; CHECK: .byte  3
+; CHECK: .byte  16
+; CHECK: .short 7
+; CHECK: .long  16
+; CHECK: .byte  3
+; CHECK: .byte  16
+; CHECK: .short 7
+; CHECK: .long  0
+
+; CHECK: .Ltmp5-test3
+; Check for the four spill slots
+; Stack Maps:     Loc 3: Indirect 7+16  [encoding: .byte 3, .byte 16, .short 7, .int 16]
+; Stack Maps:     Loc 4: Indirect 7+16  [encoding: .byte 3, .byte 16, .short 7, .int 16]
+; Stack Maps:     Loc 5: Indirect 7+16  [encoding: .byte 3, .byte 16, .short 7, .int 16]
+; Stack Maps:     Loc 6: Indirect 7+0   [encoding: .byte 3, .byte 16, .short 7, .int 0]
+; CHECK: .byte  3
+; CHECK: .byte  16
+; CHECK: .short 7
+; CHECK: .long  16
+; CHECK: .byte  3
+; CHECK: .byte  16
+; CHECK: .short 7
+; CHECK: .long  16
+; CHECK: .byte  3
+; CHECK: .byte  16
+; CHECK: .short 7
+; CHECK: .long  16
+; CHECK: .byte  3
+; CHECK: .byte  16
+; CHECK: .short 7
+; CHECK: .long  0
+
+declare void @do_safepoint()
+
+declare token @llvm.experimental.gc.statepoint.p0f_isVoidf(i64, i32, void ()*, i32, i32, ...)
+declare i8 addrspace(1)* @llvm.experimental.gc.relocate.p1i8(token, i32, i32)
+declare <2 x i8 addrspace(1)*> @llvm.experimental.gc.relocate.v2p1i8(token, i32, i32)

llvm/trunk/test/Verifier/gc_relocate_return.ll

 ; RUN: not llvm-as -disable-output <%s 2>&1 | FileCheck %s
-; This is to verify that gc_relocate must return a pointer type, which is defined
-; in intrinsics.td.
+; This is to verify that gc_relocate must return a pointer type
 
-; CHECK: Intrinsic has incorrect return type!
+; CHECK: gc.relocate must return a pointer or a vector of pointers
 
 declare void @foo()
 
 declare token @llvm.experimental.gc.statepoint.p0f_isVoidf(i64, i32, void ()*, i32, i32, ...)
 
 define void @test1(<2 x i32 addrspace(1)*> addrspace(1)* %obj) gc "statepoint-example" {
 entry:
   %safepoint_token = call token (i64, i32, void ()*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_isVoidf(i64 0, i32 0, void ()* @foo, i32 0, i32 0, i32 0, i32 0, <2 x i32 addrspace(1)*> addrspace(1)* %obj)