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

[Reid Kleckner via Phabricator via llvm-commits]

rnk 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
----------------
nickdesaulniers wrote:
> nickdesaulniers wrote:
> > rnk wrote:
> > > pengfei wrote:
> > > > 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.
> > > 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.
> > 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);
> > ```
> @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`?
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92695