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

[Phoebe Wang via Phabricator via llvm-commits]

pengfei added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:25975-25976
-      // sequence.
-      MachineFrameInfo &MFI = DAG.getMachineFunction().getFrameInfo();
-      MFI.setHasCopyImplyingStackAdjustment(true);
       // Don't do anything here, we will expand these intrinsics out later
----------------
rnk wrote:
> pengfei wrote:
> > craig.topper wrote:
> > > pengfei wrote:
> > > > How about address Craig and Reid's concerns by adding the check conditions like above?
> > > emitPrologue calls setUsesRedZone. That would occur after this code runs. This is too early to call getUsesRedZone.
> > 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);
> > ```
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92695/new/

https://reviews.llvm.org/D92695