[X86] Only force FP usage in the presence of pushf/popf on Win64

[rnk]

Summary

This ensures that the Windows unwinder will work at every instruction
boundary, and allows other targets to read and write flags without
setting up a frame pointer.

[nickdesaulniers]

llvm/lib/Target/X86/X86FrameLowering.cpp

103

isWin64Prologue(MF) &&

[rnk]

• use a helper

[nickdesaulniers]

This approach works, too (in addition to D92695) and is simpler (though, should X86FrameLowering::has128ByteRedZone now return false for frame using the intrinsics that aren't wincfi? If so, then modification to that method too will degenerate into D92695).

Consider whether D119382 still has value; if it does, perhaps it should land first and/or this patch made a child of it; otherwise happy to abandon it.

Also, in D92695, I modify llvm/test/CodeGen/X86/x86-64-flags-intrinsics.ll. I'm not sure what's preferred for that test. PTAL

It would be helpful to have in the commit message for this patch that it

Fixes: https://github.com/llvm/llvm-project/issues/46875

[rnk]

In D119391#3309645, @nickdesaulniers wrote:

This approach works, too (in addition to D92695) and is simpler (though, should X86FrameLowering::has128ByteRedZone now return false for frame using the intrinsics that aren't wincfi? If so, then modification to that method too will degenerate into D92695).

Yeah, my understanding of has128ByteRedZone is that it is meant to be a target check: does the target have a redzone by convention? So, I think it makes sense to keep the function-specific compatibility checks at the call site where it is used.

Consider whether D119382 still has value; if it does, perhaps it should land first and/or this patch made a child of it; otherwise happy to abandon it.

Now that I found the existing in tree x86-64-flags-intrinsics test, I think we could skip that new test.

Also, in D92695, I modify llvm/test/CodeGen/X86/x86-64-flags-intrinsics.ll. I'm not sure what's preferred for that test. PTAL

You know I went looking for that, but did not find it... I will update it to have it check win64 and linux codegen, since those behave differently now.

It would be helpful to have in the commit message for this patch that it

Fixes: https://github.com/llvm/llvm-project/issues/46875

Sure.

[majnemer]

Is there a test which uses flags.read with an alloca to ensure that we obey the redzone rules?

llvm/lib/Target/X86/X86FrameLowering.cpp

bool X86FrameLowering::hasFP(const MachineFunction &MF) const {
   const MachineFrameInfo &MFI = MF.getFrameInfo();
   return (MF.getTarget().Options.DisableFramePointerElim(MF) ||
           TRI->hasStackRealignment(MF) || MFI.hasVarSizedObjects() ||
           MFI.isFrameAddressTaken() || MFI.hasOpaqueSPAdjustment() ||
           MF.getInfo<X86MachineFunctionInfo>()->getForceFramePointer() ||
           MF.getInfo<X86MachineFunctionInfo>()->hasPreallocatedCall() ||
           MF.callsUnwindInit() || MF.hasEHFunclets() || MF.callsEHReturn() ||
           MFI.hasStackMap() || MFI.hasPatchPoint() ||
-          MFI.hasCopyImplyingStackAdjustment());
+          (isWin64Prologue(MF) && MFI.hasCopyImplyingStackAdjustment()));
 }
 
 static unsigned getSUBriOpcode(bool IsLP64, int64_t Imm) {
   if (IsLP64) {
     if (isInt<8>(Imm))
       return X86::SUB64ri8;
     return X86::SUB64ri32;
   } else {
bool X86FrameLowering::has128ByteRedZone(const MachineFunction& MF) const {
   // clobbered by any interrupt handler.
   assert(&STI == &MF.getSubtarget<X86Subtarget>() &&
          "MF used frame lowering for wrong subtarget");
   const Function &Fn = MF.getFunction();
   const bool IsWin64CC = STI.isCallingConvWin64(Fn.getCallingConv());
   return Is64Bit && !IsWin64CC && !Fn.hasFnAttribute(Attribute::NoRedZone);
 }
 
+/// Return true if we need to use the restricted Windows x64 prologue and
+/// epilogue code patterns that can be described with WinCFI (.seh_*
+/// directives).
 bool X86FrameLowering::isWin64Prologue(const MachineFunction &MF) const {
   return MF.getTarget().getMCAsmInfo()->usesWindowsCFI();
 }
 
 bool X86FrameLowering::needsDwarfCFI(const MachineFunction &MF) const {
   return !isWin64Prologue(MF) && MF.needsFrameMoves();
 }
 

llvm/test/CodeGen/X86/x86-64-flags-intrinsics.ll

-; RUN: llc -verify-machineinstrs < %s | FileCheck %s
-target triple = "x86_64-pc-win32"
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -verify-machineinstrs -mtriple x86_64-linux < %s | FileCheck %s
+; RUN: llc -verify-machineinstrs -mtriple x86_64-windows < %s | FileCheck %s --check-prefix=WIN64
 
 declare i64 @llvm.x86.flags.read.u64()
 declare void @llvm.x86.flags.write.u64(i64)
 
 define i64 @read_flags() {
+; CHECK-LABEL: read_flags:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    pushfq
+; CHECK-NEXT:    popq %rax
+; CHECK-NEXT:    retq
+;
+; WIN64-LABEL: read_flags:
+; WIN64:       # %bb.0: # %entry
+; WIN64-NEXT:    pushq %rbp
+; WIN64-NEXT:    .seh_pushreg %rbp
+; WIN64-NEXT:    movq %rsp, %rbp
+; WIN64-NEXT:    .seh_setframe %rbp, 0
+; WIN64-NEXT:    .seh_endprologue
+; WIN64-NEXT:    pushfq
+; WIN64-NEXT:    popq %rax
+; WIN64-NEXT:    popq %rbp
+; WIN64-NEXT:    retq
+; WIN64-NEXT:    .seh_endproc
 entry:
   %flags = call i64 @llvm.x86.flags.read.u64()
   ret i64 %flags
 }
 
-; CHECK-LABEL: read_flags:
-; CHECK:      pushq   %rbp
-; CHECK:      .seh_pushreg %rbp
-; CHECK:      movq    %rsp, %rbp
-; CHECK:      .seh_setframe %rbp, 0
-; CHECK:      .seh_endprologue
-; CHECK-NEXT: pushfq
-; CHECK-NEXT: popq    %rax
-; CHECK-NEXT: popq    %rbp
-
 define void @write_flags(i64 %arg) {
+; CHECK-LABEL: write_flags:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    pushq %rdi
+; CHECK-NEXT:    popfq
+; CHECK-NEXT:    retq
+;
+; WIN64-LABEL: write_flags:
+; WIN64:       # %bb.0: # %entry
+; WIN64-NEXT:    pushq %rbp
+; WIN64-NEXT:    .seh_pushreg %rbp
+; WIN64-NEXT:    movq %rsp, %rbp
+; WIN64-NEXT:    .seh_setframe %rbp, 0
+; WIN64-NEXT:    .seh_endprologue
+; WIN64-NEXT:    pushq %rcx
+; WIN64-NEXT:    popfq
+; WIN64-NEXT:    popq %rbp
+; WIN64-NEXT:    retq
+; WIN64-NEXT:    .seh_endproc
 entry:
   call void @llvm.x86.flags.write.u64(i64 %arg)
   ret void
 }
-
-; CHECK-LABEL: write_flags:
-; CHECK:      pushq   %rbp
-; CHECK:      .seh_pushreg %rbp
-; CHECK:      movq    %rsp, %rbp
-; CHECK:      .seh_setframe %rbp, 0
-; CHECK:      .seh_endprologue
-; CHECK-NEXT: pushq   %rcx
-; CHECK-NEXT: popfq
-; CHECK-NEXT: popq    %rbp

llvm/test/CodeGen/X86/x86-flags-intrinsics.ll

 ; RUN: llc -verify-machineinstrs < %s | FileCheck %s
 target triple = "i686-pc-win32"
 
+; Check that pushf/popf intrinsics on win32 don't need a frame pointer.
+; FIXME: These can't be autogenerated due to the fastcall function name label,
+; it seems.
+
 declare i32 @llvm.x86.flags.read.u32()
 declare void @llvm.x86.flags.write.u32(i32)
 
 define i32 @read_flags() {
 entry:
   %flags = call i32 @llvm.x86.flags.read.u32()
   ret i32 %flags
 }
 
 ; CHECK-LABEL: _read_flags:
-; CHECK:      pushl   %ebp
-; CHECK-NEXT: movl    %esp, %ebp
-; CHECK-NEXT: pushfl
+; CHECK:      pushfl
 ; CHECK-NEXT: popl    %eax
-; CHECK-NEXT: popl    %ebp
+; CHECK-NEXT: retl
 
 define x86_fastcallcc void @write_flags(i32 inreg %arg) {
 entry:
   call void @llvm.x86.flags.write.u32(i32 %arg)
   ret void
 }
 
 ; CHECK-LABEL: @write_flags@4:
-; CHECK:      pushl   %ebp
-; CHECK-NEXT: movl    %esp, %ebp
-; CHECK-NEXT: pushl   %ecx
+; CHECK:      pushl   %ecx
 ; CHECK-NEXT: popfl
-; CHECK-NEXT: popl    %ebp
+; CHECK-NEXT: retl