Use a pointer type for target frame indices during statepoint lowering

[sanjoy]

Summary

The type of the target frame index is intptr, not the type of the value we're
going to store into it. Without this change we crash in the attached test case
when trying to type-legalize a TargetFrameIndex.

Patchpoint lowering types the target frame index as intptr as well.

[arsenm]

See D32021, after that is committed this should use getFrameIndexTy

[sanjoy]

In D32256#731326, @arsenm wrote:

See D32021, after that is committed this should use getFrameIndexTy

Thanks for the tip! I'll rebase once that lands.

[reames]

Can you explain why it's correct to use the pointer type in this one call to getTargetFrameIndex and not the 4 other ones in the same file? I'm a bit confused here.

[sanjoy]

In D32256#733048, @reames wrote:

Can you explain why it's correct to use the pointer type in this one call to getTargetFrameIndex and not the 4 other ones in the same file? I'm a bit confused here.

Yes, they need to be fixed too.

[sanjoy]

• address review

[arsenm]

LGTM

lib/CodeGen/SelectionDAG/StatepointLowering.cpp

 /// emitted store
 static std::pair<SDValue, SDValue>
 spillIncomingStatepointValue(SDValue Incoming, SDValue Chain,
                              SelectionDAGBuilder &Builder) {
   SDValue Loc = Builder.StatepointLowering.getLocation(Incoming);
 
   // Emit new store if we didn't do it for this ptr before
   if (!Loc.getNode()) {
+    auto &TLI = Builder.DAG.getTargetLoweringInfo();
+    auto &DL = Builder.DAG.getDataLayout();
     Loc = Builder.StatepointLowering.allocateStackSlot(Incoming.getValueType(),
                                                        Builder);
     int Index = cast<FrameIndexSDNode>(Loc)->getIndex();
     // We use TargetFrameIndex so that isel will not select it into LEA
-    Loc = Builder.DAG.getTargetFrameIndex(Index, Incoming.getValueType());
+    Loc = Builder.DAG.getTargetFrameIndex(Index, TLI.getPointerTy(DL));
 
     // TODO: We can create TokenFactor node instead of
     //       chaining stores one after another, this may allow
     //       a bit more optimal scheduling for them
 
 #ifndef NDEBUG
     // Right now we always allocate spill slots that are of the same
     // size as the value we're about to spill (the size of spillee can

test/CodeGen/X86/deopt-bundles.ll

 ; STACKMAPS-NEXT: Stack Maps: callsite 4243
 ; STACKMAPS-NEXT: Stack Maps:   has 4 locations
 ; STACKMAPS-NEXT: Stack Maps:     Loc 0: Constant 0 [encoding: .byte 4, .byte 8, .short 0, .int 0]
 ; STACKMAPS-NEXT: Stack Maps:     Loc 1: Constant 0 [encoding: .byte 4, .byte 8, .short 0, .int 0]
 ; STACKMAPS-NEXT: Stack Maps:     Loc 2: Constant 1 [encoding: .byte 4, .byte 8, .short 0, .int 1]
 ; STACKMAPS-NEXT: Stack Maps:     Loc 3: Constant 55  [encoding: .byte 4, .byte 8, .short 0, .int 55]
 ; STACKMAPS-NEXT: Stack Maps:   has 0 live-out registers
 
-
 declare i32 @callee_0()
 declare i32 @callee_1(i32)
 declare i32 @callee_vararg(...)
 
 define i32 @caller_0() {
 ; CHECK-LABEL: _caller_0
 entry:
   %v = call i32 @callee_0() [ "deopt"(i32 0) ]
define void @f_0(i64 %n) {
   ; Check that the stackmap does not reference %s through
   ; SP since the offset is not static because of %vl.
   ; STACKMAPS: Loc 3: Direct 6
   call void @g_0(i64* %vl) [ "deopt"(i64* %s) ]
   ret void
 }
 
 declare void @g_0(i64* %vl)
+
+define void @vector_deopt_bundle(<32 x i64 addrspace(1)*> %val) {
+; CHECK-LABEL: _vector_deopt_bundle:
+; CHECK: movaps  16(%rbp), %xmm8
+; CHECK-NEXT: movaps  32(%rbp), %xmm9
+; CHECK-NEXT: movaps  48(%rbp), %xmm10
+; CHECK-NEXT: movaps  64(%rbp), %xmm11
+; CHECK-NEXT: movaps  80(%rbp), %xmm12
+; CHECK-NEXT: movaps  96(%rbp), %xmm13
+; CHECK-NEXT: movaps  112(%rbp), %xmm14
+; CHECK-NEXT: movaps  128(%rbp), %xmm15
+; CHECK-NEXT: movaps  %xmm15, 240(%rsp)
+; CHECK-NEXT: movaps  %xmm14, 224(%rsp)
+; CHECK-NEXT: movaps  %xmm13, 208(%rsp)
+; CHECK-NEXT: movaps  %xmm12, 192(%rsp)
+; CHECK-NEXT: movaps  %xmm11, 176(%rsp)
+; CHECK-NEXT: movaps  %xmm10, 160(%rsp)
+; CHECK-NEXT: movaps  %xmm9, 144(%rsp)
+; CHECK-NEXT: movaps  %xmm8, 128(%rsp)
+; CHECK-NEXT: movaps  %xmm7, 112(%rsp)
+; CHECK-NEXT: movaps  %xmm6, 96(%rsp)
+; CHECK-NEXT: movaps  %xmm5, 80(%rsp)
+; CHECK-NEXT: movaps  %xmm4, 64(%rsp)
+; CHECK-NEXT: movaps  %xmm3, 48(%rsp)
+; CHECK-NEXT: movaps  %xmm2, 32(%rsp)
+; CHECK-NEXT: movaps  %xmm1, 16(%rsp)
+; CHECK-NEXT: movaps  %xmm0, (%rsp)
+  call void @unknown() [ "deopt"(<32 x i64 addrspace(1)*> %val) ]
+  ret void
+; STACKMAPS: Stack Maps: callsite 2882400015
+; STACKMAPS-NEXT: Stack Maps:   has 4 locations
+; STACKMAPS-NEXT: Stack Maps:     Loc 0: Constant 0 [encoding: .byte 4, .byte 8, .short 0, .int 0]
+; STACKMAPS-NEXT: Stack Maps:     Loc 1: Constant 0 [encoding: .byte 4, .byte 8, .short 0, .int 0]
+; STACKMAPS-NEXT: Stack Maps:     Loc 2: Constant 1 [encoding: .byte 4, .byte 8, .short 0, .int 1]
+; STACKMAPS-NEXT: Stack Maps:     Loc 3: Indirect 7+0 [encoding: .byte 3, .byte 256, .short 7, .int 0]
+; STACKMAPS-NEXT: Stack Maps:   has 0 live-out registers
+}
+
+declare void @unknown()