MachineModuleInfo: Move HasSplitStack handling to AsmPrinter

[arsenm]

Summary

This is used to emit one field in doFinalization for the module. We
can accumulate this when emitting all individual functions directly in
the AsmPrinter, rather than accumulating additional state in
MachineModuleInfo.

Move the special case behavior predicate into MachineFrameInfo to
share it. This now promotes it to generic behavior. I'm assuming this
is fine because no other target implements adjustForSegmentedStacks,
or has tests using the split-stack attribute.

[rnk]

I read through the situation described in PR37807 the two relevant commits (rG9cc1ffadc5ad06ab846a7da95a1afb874b9f3d98 and D48444), and I think I understand the two issues.

I think it would be simpler if we tracked a boolean (HasSplitStackPrologue) on each MachineFrameInfo object indicating whether a split stack prologue was emitted or not for some particular function. This avoids the need to duplicate this stack size and tail call checking logic in the AsmPrinter.

The root of the issue here is that we are checking IR attributes in MF.shouldSplitStack(), but we allow ourselves to revise that decision during prologue emission if we have a leaf function that doesn't use any stack. The AsmPrinter should look at the MIR, not the IR, and that will simplify things.

llvm/lib/Target/ARM/ARMAsmPrinter.cpp

27

Seems unnecessary?

[arsenm]

In D123932#3457400, @rnk wrote:

I read through the situation described in PR37807 the two relevant commits (rG9cc1ffadc5ad06ab846a7da95a1afb874b9f3d98 and D48444), and I think I understand the two issues.

I think it would be simpler if we tracked a boolean (HasSplitStackPrologue) on each MachineFrameInfo object indicating whether a split stack prologue was emitted or not for some particular function. This avoids the need to duplicate this stack size and tail call checking logic in the AsmPrinter.

The root of the issue here is that we are checking IR attributes in MF.shouldSplitStack(), but we allow ourselves to revise that decision during prologue emission if we have a leaf function that doesn't use any stack. The AsmPrinter should look at the MIR, not the IR, and that will simplify things.

I tried this and it doesn't quite work. Maintaining the current behavior requires 2 fields; that split-stacks requested and whether or not it was actually emitted. Do you think it would be better to just move the StackSize == 0 && !MFI.hasTailCall() check into a helper in MFI, track a second field in MFI, or just keep having the AsmPrinter look at the attribute?

[rnk]

In D123932#3457579, @arsenm wrote:

I tried this and it doesn't quite work. Maintaining the current behavior requires 2 fields; that split-stacks requested and whether or not it was actually emitted. Do you think it would be better to just move the StackSize == 0 && !MFI.hasTailCall() check into a helper in MFI, track a second field in MFI, or just keep having the AsmPrinter look at the attribute?

I think you're right, that's the way to go. This logic is shared between targets anyway, and it gives us a place to park the long comments about the desired behavior, which is that all non-leaf functions need split stack prologues, even if they use no stack space.

[arsenm]

Move special case predicate to MFI

[rnk]

lgtm

llvm/include/llvm/CodeGen/AsmPrinter.h

219

I think this is missing a negative, maybe it can be "when it also contains functions without a split stack prologue".

[arsenm]

d7938b1a817006388f95de5ea2ee74daa7cde892

llvm/include/llvm/CodeGen/AsmPrinter.h

private:
 
   /// A handler that supports pseudo probe emission with embedded inline
   /// context.
   PseudoProbeHandler *PP = nullptr;
 
   /// CFISection type the module needs i.e. either .eh_frame or .debug_frame.
   CFISection ModuleCFISection = CFISection::None;
 
+  /// True if the module contains split-stack functions. This is used to
+  /// emit .note.GNU-split-stack section as required by the linker for
+  /// special handling split-stack function calling no-split-stack function.
+  bool HasSplitStack = false;
+
+  /// True if the module contains no-split-stack functions. This is used to
+  /// emit .note.GNU-no-split-stack section when it also contains split-stack
+  /// functions.
+  bool HasNoSplitStack = false;
+
 protected:
   explicit AsmPrinter(TargetMachine &TM, std::unique_ptr<MCStreamer> Streamer);
 
 public:
   ~AsmPrinter() override;
 
   DwarfDebug *getDwarfDebug() { return DD; }
   DwarfDebug *getDwarfDebug() const { return DD; }

llvm/include/llvm/CodeGen/MachineFrameInfo.h

public:
   void setHasStackMap(bool s = true) { HasStackMap = s; }
 
   /// This method may be called any time after instruction
   /// selection is complete to determine if there is a call to builtin
   /// \@llvm.experimental.patchpoint.
   bool hasPatchPoint() const { return HasPatchPoint; }
   void setHasPatchPoint(bool s = true) { HasPatchPoint = s; }
 
+  /// Return true if this function requires a split stack prolog, even if it
+  /// uses no stack space. This is only meaningful for functions where
+  /// MachineFunction::shouldSplitStack() returns true.
+  //
+  // For non-leaf functions we have to allow for the possibility that the call
+  // is to a non-split function, as in PR37807. This function could also take
+  // the address of a non-split function. When the linker tries to adjust its
+  // non-existent prologue, it would fail with an error. Mark the object file so
+  // that such failures are not errors. See this Go language bug-report
+  // https://go-review.googlesource.com/c/go/+/148819/
+  bool needsSplitStackProlog() const {
+    return getStackSize() != 0 || hasTailCall();
+  }
+
   /// Return the minimum frame object index.
   int getObjectIndexBegin() const { return -NumFixedObjects; }
 
   /// Return one past the maximum frame object index.
   int getObjectIndexEnd() const { return (int)Objects.size()-NumFixedObjects; }
 
   /// Return the number of fixed objects.
   unsigned getNumFixedObjects() const { return NumFixedObjects; }

llvm/include/llvm/CodeGen/MachineModuleInfo.h

class MachineModuleInfo {
   bool DbgInfoAvailable;
 
   /// True if the module calls the __morestack function indirectly, as is
   /// required under the large code model on x86. This is used to emit
   /// a definition of a symbol, __morestack_addr, containing the address. See
   /// comments in lib/Target/X86/X86FrameLowering.cpp for more details.
   bool UsesMorestackAddr;
 
-  /// True if the module contains split-stack functions. This is used to
-  /// emit .note.GNU-split-stack section as required by the linker for
-  /// special handling split-stack function calling no-split-stack function.
-  bool HasSplitStack;
-
-  /// True if the module contains no-split-stack functions. This is used to
-  /// emit .note.GNU-no-split-stack section when it also contains split-stack
-  /// functions.
-  bool HasNosplitStack;
-
   /// Maps IR Functions to their corresponding MachineFunctions.
   DenseMap<const Function*, std::unique_ptr<MachineFunction>> MachineFunctions;
   /// Next unique number available for a MachineFunction.
   unsigned NextFnNum = 0;
   const Function *LastRequest = nullptr; ///< Used for shortcut/cache.
   MachineFunction *LastResult = nullptr; ///< Used for shortcut/cache.
 
   MachineModuleInfo &operator=(MachineModuleInfo &&MMII) = delete;
public:
   bool usesMorestackAddr() const {
     return UsesMorestackAddr;
   }
 
   void setUsesMorestackAddr(bool b) {
     UsesMorestackAddr = b;
   }
 
-  bool hasSplitStack() const {
-    return HasSplitStack;
-  }
-
-  void setHasSplitStack(bool b) {
-    HasSplitStack = b;
-  }
-
-  bool hasNosplitStack() const {
-    return HasNosplitStack;
-  }
-
-  void setHasNosplitStack(bool b) {
-    HasNosplitStack = b;
-  }
-
   /// Return the symbol to be used for the specified basic block when its
   /// address is taken.  This cannot be its normal LBB label because the block
   /// may be accessed outside its containing function.
   MCSymbol *getAddrLabelSymbol(const BasicBlock *BB) {
     return getAddrLabelSymbolToEmit(BB).front();
   }
 
   /// Return the symbol to be used for the specified basic block when its

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

void AsmPrinter::getAnalysisUsage(AnalysisUsage &AU) const {
   MachineFunctionPass::getAnalysisUsage(AU);
   AU.addRequired<MachineOptimizationRemarkEmitterPass>();
   AU.addRequired<GCModuleInfo>();
 }
 
 bool AsmPrinter::doInitialization(Module &M) {
   auto *MMIWP = getAnalysisIfAvailable<MachineModuleInfoWrapperPass>();
   MMI = MMIWP ? &MMIWP->getMMI() : nullptr;
+  HasSplitStack = false;
+  HasNoSplitStack = false;
 
   // Initialize TargetLoweringObjectFile.
   const_cast<TargetLoweringObjectFile&>(getObjFileLowering())
     .Initialize(OutContext, TM);
 
   const_cast<TargetLoweringObjectFile &>(getObjFileLowering())
       .getModuleMetadata(M);
 
if (MMI->usesMorestackAddr()) {
 
     unsigned PtrSize = MAI->getCodePointerSize();
     OutStreamer->emitSymbolValue(GetExternalSymbolSymbol("__morestack"),
                                  PtrSize);
   }
 
   // Emit .note.GNU-split-stack and .note.GNU-no-split-stack sections if
   // split-stack is used.
-  if (TM.getTargetTriple().isOSBinFormatELF() && MMI->hasSplitStack()) {
+  if (TM.getTargetTriple().isOSBinFormatELF() && HasSplitStack) {
     OutStreamer->SwitchSection(
         OutContext.getELFSection(".note.GNU-split-stack", ELF::SHT_PROGBITS, 0));
-    if (MMI->hasNosplitStack())
+    if (HasNoSplitStack)
       OutStreamer->SwitchSection(
           OutContext.getELFSection(".note.GNU-no-split-stack", ELF::SHT_PROGBITS, 0));
   }
 
   // If we don't have any trampolines, then we don't require stack memory
   // to be executable. Some targets have a directive to declare this.
   Function *InitTrampolineIntrinsic = M.getFunction("llvm.init.trampoline");
   if (!InitTrampolineIntrinsic || InitTrampolineIntrinsic->use_empty())
if (Res.second)
     Res.first->second = createTempSymbol("exception");
   return Res.first->second;
 }
 
 void AsmPrinter::SetupMachineFunction(MachineFunction &MF) {
   this->MF = &MF;
   const Function &F = MF.getFunction();
 
+  // Record that there are split-stack functions, so we will emit a special
+  // section to tell the linker.
+  if (MF.shouldSplitStack()) {
+    HasSplitStack = true;
+
+    if (!MF.getFrameInfo().needsSplitStackProlog())
+      HasNoSplitStack = true;
+  } else
+    HasNoSplitStack = true;
+
   // Get the function symbol.
   if (!MAI->needsFunctionDescriptors()) {
     CurrentFnSym = getSymbol(&MF.getFunction());
   } else {
     assert(TM.getTargetTriple().isOSAIX() &&
            "Only AIX uses the function descriptor hooks.");
     // AIX is unique here in that the name of the symbol emitted for the
     // function body does not have the same name as the source function's

llvm/lib/CodeGen/MachineModuleInfo.cpp

void MMIAddrLabelMapCallbackPtr::allUsesReplacedWith(Value *V2) {
   Map->UpdateForRAUWBlock(cast<BasicBlock>(getValPtr()), cast<BasicBlock>(V2));
 }
 
 void MachineModuleInfo::initialize() {
   ObjFileMMI = nullptr;
   CurCallSite = 0;
   NextFnNum = 0;
   UsesMorestackAddr = false;
-  HasSplitStack = HasNosplitStack = false;
   AddrLabelSymbols = nullptr;
 }
 
 void MachineModuleInfo::finalize() {
   Personalities.clear();
 
   delete AddrLabelSymbols;
   AddrLabelSymbols = nullptr;
: TM(std::move(MMI.TM)),
       Context(MMI.TM.getTargetTriple(), MMI.TM.getMCAsmInfo(),
               MMI.TM.getMCRegisterInfo(), MMI.TM.getMCSubtargetInfo(), nullptr,
               nullptr, false),
       MachineFunctions(std::move(MMI.MachineFunctions)) {
   Context.setObjectFileInfo(MMI.TM.getObjFileLowering());
   ObjFileMMI = MMI.ObjFileMMI;
   CurCallSite = MMI.CurCallSite;
   UsesMorestackAddr = MMI.UsesMorestackAddr;
-  HasSplitStack = MMI.HasSplitStack;
-  HasNosplitStack = MMI.HasNosplitStack;
   AddrLabelSymbols = MMI.AddrLabelSymbols;
   ExternalContext = MMI.ExternalContext;
   TheModule = MMI.TheModule;
 }
 
 MachineModuleInfo::MachineModuleInfo(const LLVMTargetMachine *TM)
     : TM(*TM), Context(TM->getTargetTriple(), TM->getMCAsmInfo(),
                        TM->getMCRegisterInfo(), TM->getMCSubtargetInfo(),

llvm/lib/CodeGen/PrologEpilogInserter.cpp

void PEI::insertPrologEpilogCode(MachineFunction &MF) {
 
   // Emit additional code that is required to support segmented stacks, if
   // we've been asked for it.  This, when linked with a runtime with support
   // for segmented stacks (libgcc is one), will result in allocating stack
   // space in small chunks instead of one large contiguous block.
   if (MF.shouldSplitStack()) {
     for (MachineBasicBlock *SaveBlock : SaveBlocks)
       TFI.adjustForSegmentedStacks(MF, *SaveBlock);
-    // Record that there are split-stack functions, so we will emit a
-    // special section to tell the linker.
-    MF.getMMI().setHasSplitStack(true);
-  } else
-    MF.getMMI().setHasNosplitStack(true);
+  }
 
   // Emit additional code that is required to explicitly handle the stack in
   // HiPE native code (if needed) when loaded in the Erlang/OTP runtime. The
   // approach is rather similar to that of Segmented Stacks, but it uses a
   // different conditional check and another BIF for allocating more stack
   // space.
   if (MF.getFunction().getCallingConv() == CallingConv::HiPE)
     for (MachineBasicBlock *SaveBlock : SaveBlocks)

llvm/lib/Target/ARM/ARMAsmPrinter.cpp

 #include "ARMTargetObjectFile.h"
 #include "MCTargetDesc/ARMAddressingModes.h"
 #include "MCTargetDesc/ARMInstPrinter.h"
 #include "MCTargetDesc/ARMMCExpr.h"
 #include "TargetInfo/ARMTargetInfo.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/BinaryFormat/COFF.h"
+#include "llvm/CodeGen/MachineFrameInfo.h"
 #include "llvm/CodeGen/MachineFunctionPass.h"
 #include "llvm/CodeGen/MachineJumpTableInfo.h"
 #include "llvm/CodeGen/MachineModuleInfoImpls.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/Mangler.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/Type.h"

llvm/lib/Target/ARM/ARMFrameLowering.cpp

void ARMFrameLowering::adjustForSegmentedStacks(
   MachineModuleInfo &MMI = MF.getMMI();
   MCContext &Context = MMI.getContext();
   const MCRegisterInfo *MRI = Context.getRegisterInfo();
   const ARMBaseInstrInfo &TII =
       *static_cast<const ARMBaseInstrInfo *>(MF.getSubtarget().getInstrInfo());
   ARMFunctionInfo *ARMFI = MF.getInfo<ARMFunctionInfo>();
   DebugLoc DL;
 
-  uint64_t StackSize = MFI.getStackSize();
-
-  // Do not generate a prologue for leaf functions with a stack of size zero.
-  // For non-leaf functions we have to allow for the possibility that the
-  // callis to a non-split function, as in PR37807. This function could also
-  // take the address of a non-split function. When the linker tries to adjust
-  // its non-existent prologue, it would fail with an error. Mark the object
-  // file so that such failures are not errors. See this Go language bug-report
-  // https://go-review.googlesource.com/c/go/+/148819/
-  if (StackSize == 0 && !MFI.hasTailCall()) {
-    MF.getMMI().setHasNosplitStack(true);
+  if (!MFI.needsSplitStackProlog())
     return;
-  }
+
+  uint64_t StackSize = MFI.getStackSize();
 
   // Use R4 and R5 as scratch registers.
   // We save R4 and R5 before use and restore them before leaving the function.
   unsigned ScratchReg0 = ARM::R4;
   unsigned ScratchReg1 = ARM::R5;
   uint64_t AlignedStackSize;
 
   MachineBasicBlock *PrevStackMBB = MF.CreateMachineBasicBlock();

llvm/lib/Target/X86/X86FrameLowering.cpp

if (!STI.isTargetLinux() && !STI.isTargetDarwin() && !STI.isTargetWin32() &&
       !STI.isTargetDragonFly())
     report_fatal_error("Segmented stacks not supported on this platform.");
 
   // Eventually StackSize will be calculated by a link-time pass; which will
   // also decide whether checking code needs to be injected into this particular
   // prologue.
   StackSize = MFI.getStackSize();
 
-  // Do not generate a prologue for leaf functions with a stack of size zero.
-  // For non-leaf functions we have to allow for the possibility that the
-  // callis to a non-split function, as in PR37807. This function could also
-  // take the address of a non-split function. When the linker tries to adjust
-  // its non-existent prologue, it would fail with an error. Mark the object
-  // file so that such failures are not errors. See this Go language bug-report
-  // https://go-review.googlesource.com/c/go/+/148819/
-  if (StackSize == 0 && !MFI.hasTailCall()) {
-    MF.getMMI().setHasNosplitStack(true);
+  if (!MFI.needsSplitStackProlog())
     return;
-  }
 
   MachineBasicBlock *allocMBB = MF.CreateMachineBasicBlock();
   MachineBasicBlock *checkMBB = MF.CreateMachineBasicBlock();
   X86MachineFunctionInfo *X86FI = MF.getInfo<X86MachineFunctionInfo>();
   bool IsNested = false;
 
   // We need to know if the function has a nest argument only in 64 bit mode.
   if (Is64Bit)