Explicitly report runtime stack realignment in StackMap section

[reames]

Summary

This change adds code to explicitly mark a function which requires runtime stack realignment as not having a fixed frame size in the StackMap section. As it happens, this is not actually a functional change. The size that would be reported without the check is also "-1", but as far as I can tell, that's an accident. The code change makes this explicit.

If we decide not to merge the code change, adding the test case to ensure this behaviour is preserved is worthwhile.

Note: There's a separate bug in handling of stackmaps and patchpoints in functions which need dynamic frame realignment. The current code assumes that offsets can be calculated from RBP, but realigned frames must use RSP. (There's a variable gap between RBP and the spill slots.) This change set does not address that issue.

[atrick]

It looks to me like your change enforces expected behavior. It's great to have a test case. Juergen, do you agree?

Our plan was to add a per function field to the stack map to communicate the dynamic alignment to the runtime. We just haven't done it yet.

Currently, we don't support dynamic alignment, hence no AVX.

[ributzka]

Being able to communicate the fact that the stack has been dynamically aligned during runtime is one of the things we want to add in the future to enable AVX.

For now we only have an assert to verify that we use the frame register, but having this test case and an explicit check is even better.

Thanks for adding this.

LGTM

[reames]

Closed by commit rL214534 (authored by @reames).

llvm/trunk/lib/CodeGen/StackMaps.cpp

const MCExpr *CSOffsetExpr = MCBinaryExpr::CreateSub(
     MCSymbolRefExpr::Create(MILabel, OutContext),
     MCSymbolRefExpr::Create(AP.CurrentFnSym, OutContext),
     OutContext);
 
   CSInfos.push_back(CallsiteInfo(CSOffsetExpr, ID, Locations, LiveOuts));
 
   // Record the stack size of the current function.
   const MachineFrameInfo *MFI = AP.MF->getFrameInfo();
+  const TargetRegisterInfo *RegInfo = AP.MF->getTarget().getRegisterInfo();
+  const bool DynamicFrameSize = MFI->hasVarSizedObjects() ||
+    RegInfo->needsStackRealignment(*(AP.MF));
   FnStackSize[AP.CurrentFnSym] =
-    MFI->hasVarSizedObjects() ? UINT64_MAX : MFI->getStackSize();
+    DynamicFrameSize ? UINT64_MAX : MFI->getStackSize();
 }
 
 void StackMaps::recordStackMap(const MachineInstr &MI) {
   assert(MI.getOpcode() == TargetOpcode::STACKMAP && "expected stackmap");
 
   int64_t ID = MI.getOperand(0).getImm();
   recordStackMapOpers(MI, ID, std::next(MI.operands_begin(), 2),
                       MI.operands_end());

llvm/trunk/test/CodeGen/X86/stackmap.ll

 ; RUN: llc < %s -mtriple=x86_64-apple-darwin -mcpu=corei7 -disable-fp-elim | FileCheck %s
 ;
 ; Note: Print verbose stackmaps using -debug-only=stackmaps.
 
 ; CHECK-LABEL:  .section  __LLVM_STACKMAPS,__llvm_stackmaps
 ; CHECK-NEXT:  __LLVM_StackMaps:
 ; Header
 ; CHECK-NEXT:   .byte 1
 ; CHECK-NEXT:   .byte 0
 ; CHECK-NEXT:   .short 0
 ; Num Functions
-; CHECK-NEXT:   .long 15
+; CHECK-NEXT:   .long 16
 ; Num LargeConstants
 ; CHECK-NEXT:   .long 3
 ; Num Callsites
-; CHECK-NEXT:   .long 19
+; CHECK-NEXT:   .long 20
 
 ; Functions and stack size
 ; CHECK-NEXT:   .quad _constantargs
 ; CHECK-NEXT:   .quad 8
 ; CHECK-NEXT:   .quad _osrinline
 ; CHECK-NEXT:   .quad 24
 ; CHECK-NEXT:   .quad _osrcold
 ; CHECK-NEXT:   .quad 8

 ; CHECK-NEXT:   .quad _liveConstant
 ; CHECK-NEXT:   .quad 8
 ; CHECK-NEXT:   .quad _directFrameIdx
 ; CHECK-NEXT:   .quad 56
 ; CHECK-NEXT:   .quad _longid
 ; CHECK-NEXT:   .quad 8
 ; CHECK-NEXT:   .quad _clobberScratch
 ; CHECK-NEXT:   .quad 56
+; CHECK-NEXT:   .quad _needsStackRealignment
+; CHECK-NEXT:   .quad -1
 
 ; Large Constants
 ; CHECK-NEXT:   .quad   2147483648
 ; CHECK-NEXT:   .quad   4294967295
 ; CHECK-NEXT:   .quad   4294967296
 
 ; Callsites
 ; Constant arguments

 ; CHECK-NEXT:   .short  6
 ; CHECK-NEXT:   .long   -{{[0-9]+}}
 define void @clobberScratch(i32 %a) {
   tail call void asm sideeffect "nop", "~{ax},~{bx},~{cx},~{dx},~{bp},~{si},~{di},~{r8},~{r9},~{r10},~{r12},~{r13},~{r14},~{r15}"() nounwind
   tail call void (i64, i32, ...)* @llvm.experimental.stackmap(i64 16, i32 8, i32 %a)
   ret void
 }
 
+; A stack frame which needs to be realigned at runtime (to meet alignment 
+; criteria for values on the stack) does not have a fixed frame size. 
+; CHECK-LABEL:  .long L{{.*}}-_needsStackRealignment
+; CHECK-NEXT:   .short 0
+; 0 locations
+; CHECK-NEXT:   .short 0
+define void @needsStackRealignment() {
+  %val = alloca i64, i32 3, align 128
+  tail call void (...)* @escape_values(i64* %val)
+; Note: Adding any non-constant to the stackmap would fail because we
+; expected to be able to address off the frame pointer.  In a realigned
+; frame, we must use the stack pointer instead.  This is a separate bug.
+  tail call void (i64, i32, ...)* @llvm.experimental.stackmap(i64 0, i32 0)
+  ret void
+}
+declare void @escape_values(...)
+
 declare void @llvm.experimental.stackmap(i64, i32, ...)
 declare void @llvm.experimental.patchpoint.void(i64, i32, i8*, i32, ...)
 declare i64 @llvm.experimental.patchpoint.i64(i64, i32, i8*, i32, ...)