[X86ISelLowering] don't emit frame pointers for eflags intrinsics.

[nickdesaulniers]

Summary

We're looking to use builtin_ia32_readeflags_u64() and
builtin_ia32_writeeflags_u64() in the Linux kernel in place of
handwritten inline assembly. We would like to minimize the overhead of
accessing the eflags, though that is already relatively slow.

It seems that the use of these compiler builtins was always injecting
frame pointers. The Linux kernel explicitly avoids frame pointers for
x86_64 and relies on custom binary post processing for handling
unwinding (so, also not DWARF .eh_frame or .debug_frame), and makes
Clang's codegen for these builtins less competitive with GCC.

Fixes pr/47531.

[craig.topper]

What prevents overwriting something in the red zone?

[nickdesaulniers]

In D92695#2434973, @craig.topper wrote:

What prevents overwriting something in the red zone?

I don't follow. How does the kernel prevent overwriting something in the red zone? I don't know, https://lore.kernel.org/lkml/[e-mail]/ was the brief conversation we had on LKML, but I'd have to talk with the x86 maintainers who would know more. Andy Lutomirski seems to imply the kernel may be broken in that regard.

But I also think that is orthogonal to these builtins and frame pointers, or at least I fail to see the relation.

[craig.topper]

In D92695#2434996, @nickdesaulniers wrote:

In D92695#2434973, @craig.topper wrote:

What prevents overwriting something in the red zone?

I don't follow. How does the kernel prevent overwriting something in the red zone? I don't know, https://lore.kernel.org/lkml/[e-mail]/ was the brief conversation we had on LKML, but I'd have to talk with the x86 maintainers who would know more. Andy Lutomirski seems to imply the kernel may be broken in that regard.

But I also think that is orthogonal to these builtins and frame pointers, or at least I fail to see the relation.

When the red zone is in use, the stack pointer isn't updated to include it. So when the pushfq happens it will push a value into an address that is part of the red zone potentially clobbering something. The setHasCopyImplyingStackAdjust disables the use of the red zone for the function which avoids this issue.

Maybe the linux kernel disables the red zone and this isn't a problem. But I worry about others using these builtins without disabling the red zone.

[nickdesaulniers]

In D92695#2435025, @craig.topper wrote:

Maybe the linux kernel disables the red zone and this isn't a problem.

The x86_64 (64b) Linux kernel is built with -mno-red-zone; I'm not sure if that's precisely the same as "disables the red zone."

But I worry about others using these builtins without disabling the red zone.

Wouldn't -fno-omit-frame-pointer or /Oy- as described in this commit message prevent that? This patch is mostly about matching GCCs behavior for these builtins, so "others using these builtins without disabling the red zone" would already be broken in such a case when built with GCC.

[nickdesaulniers]

In D92695#2435063, @nickdesaulniers wrote:

In D92695#2435025, @craig.topper wrote:

Maybe the linux kernel disables the red zone and this isn't a problem.

The x86_64 (64b) Linux kernel is built with -mno-red-zone; I'm not sure if that's precisely the same as "disables the red zone."

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6ba0efa46047936afa81460489cfd24bc95dd863 has a little more context about the kernel's constraints. https://stackoverflow.com/questions/25787408/why-cant-kernel-code-use-a-red-zone has more interesting discussion as well.

[craig.topper]

In D92695#2435063, @nickdesaulniers wrote:

In D92695#2435025, @craig.topper wrote:

Maybe the linux kernel disables the red zone and this isn't a problem.

The x86_64 (64b) Linux kernel is built with -mno-red-zone; I'm not sure if that's precisely the same as "disables the red zone."

Yes that's what I meant.

But I worry about others using these builtins without disabling the red zone.

Wouldn't -fno-omit-frame-pointer or /Oy- as described in this commit message prevent that? This patch is mostly about matching GCCs behavior for these builtins, so "others using these builtins without disabling the red zone" would already be broken in such a case when built with GCC.

Yes -fno-omit-frame-pointer would fix it. gcc seems to handle the redzone correctly when readeflags is present. Compare the two functions here, https://godbolt.org/z/j7hdYY. With the readeflags builtin, there is a stack pointer adjustment near the beginning. In the second function there is no stack pointer adjustment. Adding -mno-red-zone will put a stack adjustment in the second function.

So I think this patch breaks a case that works with clang and gcc today. I don't think requiring -fno-omit-frame-pointer to use the builtins with clang is a good solution.

[majnemer]

It is difficult remembering exactly what my thought process was way back when but the redzone calculation seems very likely...

Lets see...

011980cd50d5ddc5112c8440ffe9161de60b40ae was where I first implemented these intrinsics.

Looking at the Win64 ABI, there is this bit:

A leaf function is one that does not require a function table entry. It can't make changes to any nonvolatile registers, including RSP, which means that it can't call any functions or allocate stack space.

Using these intrinsics must not violate this rule of the ABI so I forced frame pointers across all platforms.

The Linux ABI for x86-64 says that there are 128-bytes reserved for leaf functions. IIRC, forcing a frame pointer here also handled some complicated accounting issue of when or how much redzone we had to play with because this intrinsic allocates some stack...

+1 to @craig.topper's comments.

[rnk]

llvm/lib/Target/X86/X86ISelLowering.cpp

25976

This is the only writer of HasCopyImplyingStackAdjustment, so if we did go ahead with this, I'd ask you to clean up the readers.

llvm/test/CodeGen/X86/win64_frame.ll

194

From a Windows PoV, this test case needs to remain unchanged, otherwise we break the contract that stack unwinding works at every PC.

[pengfei]

llvm/lib/Target/X86/X86ISelLowering.cpp

25975–25976

How about address Craig and Reid's concerns by adding the check conditions like above?

[craig.topper]

llvm/lib/Target/X86/X86ISelLowering.cpp

25975–25976

emitPrologue calls setUsesRedZone. That would occur after this code runs. This is too early to call getUsesRedZone.

[pengfei]

llvm/lib/Target/X86/X86ISelLowering.cpp

25975–25976

Maybe we can check for -mno-red-zone only given Linux kernel is built with it:

MachineFunction &MF = DAG.getMachineFunction();
if (!MF.getFunction().hasFnAttribute(Attribute::NoRedZone))
  MF.getFrameInfo().setHasCopyImplyingStackAdjustment(true);

[rnk]

llvm/lib/Target/X86/X86ISelLowering.cpp

25975–25976

I would try to tackle this the other way around: usage of readflags/writeflags should prevent PEI from enabling the redzone optimization.

I think what we really want here is to tweak X86FrameLowering::hasFP.
If WinCFI is being used, where we have per-instruction accurate CFI, we need to force the use of a frame pointer.
For Linux, this flag should prevent the use of a red zone later in X86FrameLowering::emitProlog. We don't need to force FP usage if WinCFI is not in use.

[pengfei]

llvm/lib/Target/X86/X86ISelLowering.cpp

25975–25976

We can't check hasWinCFI here due to the same reason as getUsesRedZone, I think we can check isOSWindows instead.
Regarding red zone, I think it is a great idea. So the code can be

--- a/llvm/lib/Target/X86/X86ISelLowering.cpp
+++ b/llvm/lib/Target/X86/X86ISelLowering.cpp
@@ -27148,8 +27148,11 @@ static SDValue LowerINTRINSIC_W_CHAIN(SDValue Op, const X86Subtarget &Subtarget,
     case llvm::Intrinsic::x86_flags_write_u64: {
       // We need a frame pointer because this will get lowered to a PUSH/POP
       // sequence.
-      MachineFrameInfo &MFI = DAG.getMachineFunction().getFrameInfo();
-      MFI.setHasCopyImplyingStackAdjustment(true);
+      MachineFunction &MF = DAG.getMachineFunction();
+      if (Subtarget.getTargetTriple().isOSWindows())
+        MF.getFrameInfo().setHasCopyImplyingStackAdjustment(true);
+      else
+        MF.getFunction().addFnAttr(Attribute::NoRedZone);
       // Don't do anything here, we will expand these intrinsics out later
       // during FinalizeISel in EmitInstrWithCustomInserter.
       return Op;

I checked it with latest xmain. No test will be affected.

[void]

This comment was removed by void.

[rnk]

llvm/lib/Target/X86/X86ISelLowering.cpp

25975–25976

Right, I didn't mean that this code should check if CFI has been emitted, but whether WinCFI should be used. I believe this information is carried on MCAsmInfo. The proposed triple check is incorrect, I don't think we need to force FP usage on 32-bit Windows, it's really specific to Win64. Please use the wincfi check, it is more self-documenting.

[nickdesaulniers]

• rebase, check for wincfi

[nickdesaulniers]

llvm/lib/Target/X86/X86ISelLowering.cpp

25975–25976

I think the red zone concerns are handled correctly already...

The only caller of X86MachineFunctionInfo::setUsesRedZone is guarded by a call to X86FrameLowering::has128ByteRedZone, which checks for the noredzone fn attr:

return Is64Bit && !IsWin64CC && !Fn.hasFnAttribute(Attribute::NoRedZone);

[nickdesaulniers]

llvm/lib/Target/X86/X86ISelLowering.cpp

25975–25976

@rnk did you still want me to look into modifying X86FrameLowering::hasFP, or is that comment stale and this approach ok?

@craig.topper are there some redzone tests in tree that we should perhaps add calls to @llvm.x86.flags.{read|write}.u64?

[rnk]

llvm/lib/Target/X86/X86ISelLowering.cpp

25975–25976

The only caller of X86MachineFunctionInfo::setUsesRedZone is guarded by a call to X86FrameLowering::has128ByteRedZone, which checks for the noredzone fn attr:

Sure, but I don't think anything sets that attr except -mno-redzone. We still need to disable the use of the redzone in the presence of pushf/popf instructions. At this point, HasCopyImplyingStackAdjustment means exactly that.

@rnk did you still want me to look into modifying X86FrameLowering::hasFP, or is that comment stale and this approach ok?

Yes, I think it would be cleaner to have this code, which sets the flag, to be target-independent (it just marks functions with pushfq/popfq). hasFP should return true for wincfi targets in the presence of these instructions. The emitProlog code which decides to use a redzone should also check this flag if it doesn't already.

[nickdesaulniers]

• rebase on test from @topperc, base fix on Phoebe's suggestion

[craig.topper]

llvm/lib/Target/X86/X86ISelLowering.cpp

25975

Do we have precedent for modifying an IR function from inside of a MachineFunctionPass.

[void]

llvm/lib/Target/X86/X86ISelLowering.cpp

25975

I believe it's generally frowned upon.

[nickdesaulniers]

• rerun update_llc_test_checks.py w/ --no_x86_scrub_sp

[nickdesaulniers]

llvm/lib/Target/X86/X86ISelLowering.cpp

25975

If the tests look good and everyone's happy with the tests, then I can find another way to plumb this.

[nickdesaulniers]

• check for isWin64Prologue when checking hasCopyImplyingStackAdjustment instead of modifying IR from machine pass

[nickdesaulniers]

Looks like @rnk is much faster than me: https://reviews.llvm.org/D119391.

llvm/lib/Target/X86/X86Subtarget.h

640 ↗

(On Diff #407350)

can be dropped now

[nickdesaulniers]

• drop change to X86Subtarget

[nickdesaulniers]

prefer D119391

llvm/lib/Target/X86/X86ISelLowering.cpp

case llvm::Intrinsic::x86_wrpkru: {
                          Op.getOperand(0), Op.getOperand(2),
                          DAG.getConstant(0, dl, MVT::i32),
                          DAG.getConstant(0, dl, MVT::i32));
     }
     case llvm::Intrinsic::x86_flags_read_u32:
     case llvm::Intrinsic::x86_flags_read_u64:
     case llvm::Intrinsic::x86_flags_write_u32:
     case llvm::Intrinsic::x86_flags_write_u64: {
-      // We need a frame pointer because this will get lowered to a PUSH/POP
-      // sequence.
-      MachineFrameInfo &MFI = DAG.getMachineFunction().getFrameInfo();
-      MFI.setHasCopyImplyingStackAdjustment(true);
       // Don't do anything here, we will expand these intrinsics out later
       // during FinalizeISel in EmitInstrWithCustomInserter.
       return Op;
     }
     case Intrinsic::x86_lwpins32:
     case Intrinsic::x86_lwpins64:
     case Intrinsic::x86_umwait:
     case Intrinsic::x86_tpause: {
       SDLoc dl(Op);
       SDValue Chain = Op->getOperand(0);
       SDVTList VTs = DAG.getVTList(MVT::i32, MVT::Other);

llvm/test/CodeGen/X86/win64_frame.ll

; CHECK-NEXT:    .seh_endproc
   %gep = getelementptr [300 x i8], [300 x i8]* %alloca, i32 0, i32 0
   call void @external(i8* %gep)
   ret i32 %e
 }
 
 define i64 @f9() {
 ; CHECK-LABEL: f9:
 ; CHECK:       # %bb.0: # %entry
-; CHECK-NEXT:    pushq %rbp
-; CHECK-NEXT:    .seh_pushreg %rbp
-; CHECK-NEXT:    movq %rsp, %rbp
-; CHECK-NEXT:    .seh_setframe %rbp, 0
-; CHECK-NEXT:    .seh_endprologue
 ; CHECK-NEXT:    pushfq
 ; CHECK-NEXT:    popq %rax
-; CHECK-NEXT:    popq %rbp
 ; CHECK-NEXT:    retq
-; CHECK-NEXT:    .seh_endproc
 entry:
   %call = call i64 @llvm.x86.flags.read.u64()
   ret i64 %call
 }
 
 declare i64 @dummy()
 
 define i64 @f10(i64* %foo, i64 %bar, i64 %baz) {

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

 ; RUN: llc -verify-machineinstrs < %s | FileCheck %s
 target triple = "x86_64-pc-win32"
 
 declare i64 @llvm.x86.flags.read.u64()
 declare void @llvm.x86.flags.write.u64(i64)
 
 define i64 @read_flags() {
 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:      pushfq
 ; CHECK-NEXT: popq    %rax
-; CHECK-NEXT: popq    %rbp
 
 define void @write_flags(i64 %arg) {
 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:      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"
 
 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
 
 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