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

llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h

public:
   void LowerDeoptimizeCall(const CallInst *CI);
   void LowerDeoptimizingReturn();
 
   void LowerCallSiteWithDeoptBundleImpl(ImmutableCallSite CS, SDValue Callee,
                                         const BasicBlock *EHPadBB,
                                         bool VarArgDisallowed,
                                         bool ForceVoidReturnTy);
 
+  /// Returns the type of FrameIndex and TargetFrameIndex nodes.
+  MVT getFrameIndexTy() {
+    return DAG.getTargetLoweringInfo().getFrameIndexTy(DAG.getDataLayout());
+  }
+
 private:
   // Terminator instructions.
   void visitRet(const ReturnInst &I);
   void visitBr(const BranchInst &I);
   void visitSwitch(const SwitchInst &I);
   void visitIndirectBr(const IndirectBrInst &I);
   void visitUnreachable(const UnreachableInst &I);
   void visitCleanupRet(const CleanupReturnInst &I);

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

if (Builder.StatepointLowering.isStackSlotAllocated(Offset)) {
     // but gc state doesn't between two calls.
     return;
   }
   // Reserve this stack slot
   Builder.StatepointLowering.reserveStackSlot(Offset);
 
   // Cache this slot so we find it when going through the normal
   // assignment loop.
-  SDValue Loc = Builder.DAG.getTargetFrameIndex(*Index, Incoming.getValueType());
+  SDValue Loc =
+      Builder.DAG.getTargetFrameIndex(*Index, Builder.getFrameIndexTy());
   Builder.StatepointLowering.setLocation(Incoming, Loc);
 }
 
 /// Remove any duplicate (as SDValues) from the derived pointer pairs.  This
 /// is not required for correctness.  It's purpose is to reduce the size of
 /// StackMap section.  It has no effect on the number of spill slots required
 /// or the actual lowering.
 static void
spillIncomingStatepointValue(SDValue Incoming, SDValue Chain,
   SDValue Loc = Builder.StatepointLowering.getLocation(Incoming);
 
   // Emit new store if we didn't do it for this ptr before
   if (!Loc.getNode()) {
     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, Builder.getFrameIndexTy());
 
     // 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
if (ConstantSDNode *C = dyn_cast<ConstantSDNode>(Incoming)) {
     // pointers and other constant pointers in GC states.  Note the constant
     // vectors do not appear to actually hit this path and that anything larger
     // than an i64 value (not type!) will fail asserts here.
     pushStackMapConstant(Ops, Builder, C->getSExtValue());
   } else if (FrameIndexSDNode *FI = dyn_cast<FrameIndexSDNode>(Incoming)) {
     // This handles allocas as arguments to the statepoint (this is only
     // really meaningful for a deopt value.  For GC, we'd be trying to
     // relocate the address of the alloca itself?)
+    assert(Incoming.getValueType() == Builder.getFrameIndexTy() &&
+           "Incoming value is a frame index!");
     Ops.push_back(Builder.DAG.getTargetFrameIndex(FI->getIndex(),
-                                                  Incoming.getValueType()));
+                                                  Builder.getFrameIndexTy()));
   } else if (LiveInOnly) {
     // If this value is live in (not live-on-return, or live-through), we can
     // treat it the same way patchpoint treats it's "live in" values.  We'll 
     // end up folding some of these into stack references, but they'll be 
     // handled by the register allocator.  Note that we do not have the notion
     // of a late use so these values might be placed in registers which are 
     // clobbered by the call.  This is fine for live-in.
     Ops.push_back(Incoming);
#endif
   // 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 : SI.GCArgs) {
     SDValue Incoming = Builder.getValue(V);
     if (FrameIndexSDNode *FI = dyn_cast<FrameIndexSDNode>(Incoming)) {
       // This handles allocas as arguments to the statepoint
+      assert(Incoming.getValueType() == Builder.getFrameIndexTy() &&
+             "Incoming value is a frame index!");
       Ops.push_back(Builder.DAG.getTargetFrameIndex(FI->getIndex(),
-                                                    Incoming.getValueType()));
+                                                    Builder.getFrameIndexTy()));
     }
   }
 
   // Record computed locations for all lowered values.
   // This can not be embedded in lowering loops as we need to record *all*
   // values, while previous loops account only values with unique SDValues.
   const Instruction *StatepointInstr = SI.StatepointInstr;
   auto &SpillMap = Builder.FuncInfo.StatepointSpillMaps[StatepointInstr];
#endif
 
   // We didn't need to spill these special cases (constants and allocas).
   // See the handling in spillIncomingValueForStatepoint for detail.
   if (!DerivedPtrLocation) {
     setValue(&Relocate, SD);
     return;
   }
 
-  SDValue SpillSlot = DAG.getTargetFrameIndex(*DerivedPtrLocation,
-                                              SD.getValueType());
+  SDValue SpillSlot =
+      DAG.getTargetFrameIndex(*DerivedPtrLocation, getFrameIndexTy());
 
   // Be conservative: flush all pending loads
   // TODO: Probably we can be less restrictive on this,
   // it may allow more scheduling opportunities.
   SDValue Chain = getRoot();
 
   SDValue SpillLoad =
-      DAG.getLoad(SpillSlot.getValueType(), getCurSDLoc(), Chain, SpillSlot,
+      DAG.getLoad(DAG.getTargetLoweringInfo().getValueType(DAG.getDataLayout(),
+                                                           Relocate.getType()),
+                  getCurSDLoc(), Chain, SpillSlot,
                   MachinePointerInfo::getFixedStack(DAG.getMachineFunction(),
                                                     *DerivedPtrLocation));
 
   // Again, be conservative, don't emit pending loads
   DAG.setRoot(SpillLoad.getValue(1));
 
   assert(SpillLoad.getNode());
   setValue(&Relocate, SpillLoad);

llvm/trunk/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()