[win][x64] Allow push/pop for stack alloc when unwind v2 is required #153621
Conversation
|
@llvm/pr-subscribers-backend-x86 Author: Daniel Paoliello (dpaoliello) ChangesWhile attempting to enable Windows x64 unwind v2, compilation failed with the following error: I traced this down to an optimization in llvm-project/llvm/lib/Target/X86/X86FrameLowering.cpp Lines 324 to 340 in 6961139 Technically, using One other side effect of this change is that the stack is now allowed to be adjusted across multiple instructions, which would be needed for extremely large stack frames. Full diff: https://github.com/llvm/llvm-project/pull/153621.diff 2 Files Affected:
diff --git a/llvm/lib/Target/X86/X86WinEHUnwindV2.cpp b/llvm/lib/Target/X86/X86WinEHUnwindV2.cpp
index e9081a4ae4e72..8efb3bf6ed8a5 100644
--- a/llvm/lib/Target/X86/X86WinEHUnwindV2.cpp
+++ b/llvm/lib/Target/X86/X86WinEHUnwindV2.cpp
@@ -201,11 +201,6 @@ bool X86WinEHUnwindV2::runOnMachineFunction(MachineFunction &MF) {
"The epilog is deallocating a stack "
"allocation, but the prolog did "
"not allocate one");
- if (HasStackDealloc)
- return rejectCurrentFunctionInternalError(
- MF, Mode,
- "The epilog is deallocating the stack "
- "allocation more than once");
if (PoppedRegCount > 0)
llvm_unreachable(
"Should have raised an error: either popping before "
@@ -219,33 +214,41 @@ bool X86WinEHUnwindV2::runOnMachineFunction(MachineFunction &MF) {
case X86::POP64r:
if (State == FunctionState::InEpilog) {
- // After the stack pointer has been adjusted, the epilog must
- // POP each register in reverse order of the PUSHes in the prolog.
- PoppedRegCount++;
- if (HasStackAlloc != HasStackDealloc)
- return rejectCurrentFunctionInternalError(
- MF, Mode,
- "Cannot pop registers before the stack "
- "allocation has been deallocated");
- if (PoppedRegCount > PushedRegs.size())
- return rejectCurrentFunctionInternalError(
- MF, Mode,
- "The epilog is popping more registers than the prolog pushed");
- if (PushedRegs[PushedRegs.size() - PoppedRegCount] !=
- MI.getOperand(0).getReg())
- return rejectCurrentFunctionInternalError(
- MF, Mode,
- "The epilog is popping a registers in "
- "a different order than the "
- "prolog pushed them");
-
- // Unwind v2 records the size of the epilog not from where we place
- // SEH_BeginEpilogue (as that contains the instruction to adjust the
- // stack pointer) but from the first POP instruction (if there is
- // one).
- if (!UnwindV2StartLocation) {
- assert(PoppedRegCount == 1);
- UnwindV2StartLocation = &MI;
+ Register Reg = MI.getOperand(0).getReg();
+ if (HasStackAlloc && (PoppedRegCount == 0) &&
+ !llvm::is_contained(PushedRegs, Reg)) {
+ // If this is a pop that doesn't correspond to the set of pushed
+ // registers, then assume it was used to adjust the stack pointer.
+ HasStackDealloc = true;
+ } else {
+ // After the stack pointer has been adjusted, the epilog must
+ // POP each register in reverse order of the PUSHes in the prolog.
+ PoppedRegCount++;
+ if (HasStackAlloc != HasStackDealloc)
+ return rejectCurrentFunctionInternalError(
+ MF, Mode,
+ "Cannot pop registers before the stack "
+ "allocation has been deallocated");
+ if (PoppedRegCount > PushedRegs.size())
+ return rejectCurrentFunctionInternalError(
+ MF, Mode,
+ "The epilog is popping more registers than the prolog "
+ "pushed");
+ if (PushedRegs[PushedRegs.size() - PoppedRegCount] != Reg)
+ return rejectCurrentFunctionInternalError(
+ MF, Mode,
+ "The epilog is popping a registers in "
+ "a different order than the "
+ "prolog pushed them");
+
+ // Unwind v2 records the size of the epilog not from where we place
+ // SEH_BeginEpilogue (as that contains the instruction to adjust the
+ // stack pointer) but from the first POP instruction (if there is
+ // one).
+ if (!UnwindV2StartLocation) {
+ assert(PoppedRegCount == 1);
+ UnwindV2StartLocation = &MI;
+ }
}
} else if (State == FunctionState::FinishedEpilog)
// Unexpected instruction after the epilog.
diff --git a/llvm/test/CodeGen/X86/win64-eh-unwindv2-push-pop-stack-alloc.ll b/llvm/test/CodeGen/X86/win64-eh-unwindv2-push-pop-stack-alloc.ll
new file mode 100644
index 0000000000000..c6f76d00ce4f1
--- /dev/null
+++ b/llvm/test/CodeGen/X86/win64-eh-unwindv2-push-pop-stack-alloc.ll
@@ -0,0 +1,80 @@
+; RUN: llc -mtriple=x86_64-unknown-windows-msvc -o - %s | FileCheck %s
+
+; Regression test for Win x64 unwind v2: in some cases it is better to use
+; push+pop to adjust the stack, rather than sub+add. This is permitted with
+; unwind v2 as the requirement is that the epilog finishes adjusting the stack
+; before popping the registers listed in the unwind table.
+
+; Pushes and pops the same register.
+define void @push_pop_same() {
+ %small_alloca = alloca i32
+ ret void
+}
+; CHECK-LABEL: push_pop_same:
+; CHECK: .seh_unwindversion 2
+; CHECK-NEXT: pushq %rax
+; CHECK-NEXT: .seh_stackalloc 8
+; CHECK-NEXT: .seh_endprologue
+; CHECK-NEXT: .seh_startepilogue
+; CHECK-NEXT: popq %rax
+; CHECK-NEXT: .seh_unwindv2start
+; CHECK-NEXT: .seh_endepilogue
+; CHECK-NEXT: retq
+; CHECK-NEXT: .seh_endproc
+
+; Pushes and pops a different register.
+define i32 @push_pop_different(i32 %x, i32 %y) {
+ %small_alloca = alloca i32
+ %sum = add i32 %x, %y
+ ret i32 %sum
+}
+; CHECK-LABEL: push_pop_different:
+; CHECK: .seh_unwindversion 2
+; CHECK-NEXT: pushq %rax
+; CHECK-NEXT: .seh_stackalloc 8
+; CHECK-NEXT: .seh_endprologue
+; CHECK: .seh_startepilogue
+; CHECK-NEXT: popq %rcx
+; CHECK-NEXT: .seh_unwindv2start
+; CHECK-NEXT: .seh_endepilogue
+; CHECK-NEXT: retq
+; CHECK-NEXT: .seh_endproc
+
+; Pushes in the prolog, adds in the epilog.
+define void @push_add(ptr %a, ptr %b, ptr %out) {
+ %small_alloca = alloca i32
+ %av = load i256, ptr %a
+ %bv = load i256, ptr %b
+ %r = mul i256 %av, %bv
+ store i256 %r, ptr %out
+ ret void
+}
+; CHECK-LABEL: push_add:
+; CHECK: .seh_unwindversion 2
+; CHECK-NEXT: pushq %r15
+; CHECK-NEXT: .seh_pushreg %r15
+; CHECK-NEXT: pushq %r14
+; CHECK-NEXT: .seh_pushreg %r14
+; CHECK-NEXT: pushq %rsi
+; CHECK-NEXT: .seh_pushreg %rsi
+; CHECK-NEXT: pushq %rdi
+; CHECK-NEXT: .seh_pushreg %rdi
+; CHECK-NEXT: pushq %rbx
+; CHECK-NEXT: .seh_pushreg %rbx
+; CHECK-NEXT: pushq %rax
+; CHECK-NEXT: .seh_stackalloc 8
+; CHECK-NEXT: .seh_endprologue
+; CHECK: .seh_startepilogue
+; CHECK-NEXT: addq $8, %rsp
+; CHECK-NEXT: .seh_unwindv2start
+; CHECK-NEXT: popq %rbx
+; CHECK-NEXT: popq %rdi
+; CHECK-NEXT: popq %rsi
+; CHECK-NEXT: popq %r14
+; CHECK-NEXT: popq %r15
+; CHECK-NEXT: .seh_endepilogue
+; CHECK-NEXT: retq
+; CHECK-NEXT: .seh_endproc
+
+!llvm.module.flags = !{!0}
+!0 = !{i32 1, !"winx64-eh-unwindv2", i32 2}
|
dpaoliello
force-pushed
the
fixunwindv2
branch
2 times, most recently
from
9da36d2 to
f578766
Compare
There was a problem hiding this comment.
With this, the remaining use of HasStackDealloc is the HasStackAlloc != HasStackDealloc checks. Which is maybe fine? I guess we don't care if the number of allocation and deallocation instructions matches.
Do we need to change the llvm_unreachable in the PoppedRegCount > 0 check to rejectCurrentFunctionInternalError?
There was a problem hiding this comment.
I'd like to leave the HasStackAlloc != HasStackDealloc checks: we should see some sort of stack adjustment before popping registers and the end of the function.
The PoppedRegCount > 0 check is still correct: we only increment PoppedRegCount if it's popping one of the SEH_PushReg pushed registers, not the stack adjustment pops.
There was a problem hiding this comment.
Say you have a push and a stackalloc in the prologue. Then you have a stackdealloc, a pop, and another stackdealloc in the epilogue. We want to rejectCurrentFunctionInternalError in that case. (Not sure how likely that is, but could happen with a callee-pop calling convention.)
There was a problem hiding this comment.
Ah, I see what you mean now. Switch it to be an error and added tests.
There was a problem hiding this comment.
This is probably less likely to need future changes if you make it an MIR test, since you don't care what the original IR looks like here.
dpaoliello
force-pushed
the
fixunwindv2
branch
from
f578766 to
5223af5
Compare
While attempting to enable Windows x64 unwind v2, compilation failed with the following error: ``` fatal error: error in backend: Windows x64 Unwind v2 is required, but LLVM has generated incompatible code in function '<redacted>': Cannot pop registers before the stack allocation has been deallocated ``` I traced this down to an optimization in `X86FrameLowering`: <https://github.com/llvm/llvm-project/blob/6961139ce9154d03c88b8d46c8742a1eaa569cd9/llvm/lib/Target/X86/X86FrameLowering.cpp#L324-L340> Technically, using `push`/`pop` to adjust the stack is permitted under unwind v2: the requirement for a "canonical" epilog is that the stack is fully adjusted before the registers listed as pushed in the unwind table are popped. So, as long as the `.seh_unwindv2start` pseudo is after the pops that adjust the stack, then everything will work correctly. One other side effect of this change is that the stack is now allowed to be adjusted across multiple instructions, which would be needed for extremely large stack frames.
3 participants
While attempting to enable Windows x64 unwind v2, compilation failed with the following error:
I traced this down to an optimization in
X86FrameLowering:llvm-project/llvm/lib/Target/X86/X86FrameLowering.cpp
Lines 324 to 340 in 6961139
Technically, using
push/popto adjust the stack is permitted under unwind v2: the requirement for a "canonical" epilog is that the stack is fully adjusted before the registers listed as pushed in the unwind table are popped. So, as long as the.seh_unwindv2startpseudo is after the pops that adjust the stack, then everything will work correctly.One other side effect of this change is that the stack is now allowed to be adjusted across multiple instructions, which would be needed for extremely large stack frames.