[AIX] Replace EmitFunctionDescriptor with EmitFunctionEntryLabel() and delete needsFunctionDescriptors()

[MaskRay]

Summary

AMDGPUAsmPrinter, ARMAsmPrinter, MipsAsmPrinter, NVPTXAsmPrinter, PPCLinuxAsmPrinter, and XCoreAsmPrinter derive EmitFunctionEntryLabel() and have custom logic there. PPCAIX should not be an exception.

[merge_guards_bot]

Unit tests: pass. 61999 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[Xiangling_L]

Thank you for mentioning this issue, the reason why we added MAI->needsFunctionDescriptors() are as follows:

1. On AIX, function descriptor and function entry point are totally two different things, we are emitting two distinct symbols. Function descriptor symbol has no relation to function entry point symbol other than referencing its address. The way ELFV1 ABI handles descriptors(with the entry point being a local symbol, and the descriptor being the global symbol) it kinda squeaks into the definition of EmitFunctionEntryLabel().

2. From the description of the MCAsmInfo class:

This class is intended to be used as a base class for asm properties and features specific to the target.

Adding a hook to MAI for descriptors and querying that is a valid way to represent this. That said, there are other places in this file that change the codegen based on explicit check of either the OS or the binary format. Besides, why we prefer the way my previous patch is doing is also because needsFunctionDescriptors() makes the code self-documenting as opposed to getTargetTriple().isOSAIX().

[sfertile]

In D66724, @MaskRay wrote:

I am not sure this "pushing past the limits of what the comment intended" was a strong argument for EmitFunctionDescriptor and needsFunctionDescriptors(). This is the way all other targets (AMDGPUAsmPrinter, ARMAsmPrinter, MipsAsmPrinter, NVPTXAsmPrinter, PPCLinuxAsmPrinter, and XCoreAsmPrinter) handle things. AIX does not have to be an exception.

For AMDGPU, ARM, Mips and XCore the AsmPrinter is doing something directly related to the entry label/function symbol in the hook: like setting its type or emitting directives that directly affect the label. PowerPC32 and ELFV2 PowerPC64 embed some data just before the entry label but still in the same section. PowerPC AIX and PowerPC ELFV1 differ in that they create and emit a second symbol into a different section, that is distinct from the functions entry label. I can understand why the ELFV1 implementation chose to do it in the EmitEntryLabel callback at the time: it works and its non-intrusive, but I think the implementation approach taken in D66742 is superior.

In D66724, @MaskRay wrote:

If needsFunctionDescriptors() is really AIX specific, I feel that TM.getTargetTriple().isOSAIX() will made a casual reader's life simpler. I did not know what function descriptors are and jumped over hoops to know this is really AIX specific. We can add the abstraction when more targets use it.

I don't think its all that difficult to find out this is AIX specific. A quick grep for NeedsFunctionDescriptors will show its the only platform to set it to true, and I agree with @Xiangling_L reasoning above for why a new MCAsmInfo hook was used over directly checking if the OS is AIX. I think we have failed with describing what descriptors are and who they are useful to in the code where MAI->needsFunctionDescriptors() is used. I am happy to post a patch to fix that. I think rather then removing the abstraction though, it would be better to migrate the PowerPC ELFV1 abi to using it as well, although it might take some tweaks because the ELF and XCOFF implementations differ slightly.

llvm/include/llvm/CodeGen/AsmPrinter.h

virtual void EmitInstruction(const MachineInstr *) {
     llvm_unreachable("EmitInstruction not implemented");
   }
 
   /// Return the symbol for the specified constant pool entry.
   virtual MCSymbol *GetCPISymbol(unsigned CPID) const;
 
   virtual void EmitFunctionEntryLabel();
 
-  virtual void EmitFunctionDescriptor() {
-    llvm_unreachable("Function descriptor is target-specific.");
-  }
-
   virtual void EmitMachineConstantPoolValue(MachineConstantPoolValue *MCPV);
 
   /// Targets can override this to change how global constants that are part of
   /// a C++ static/global constructor list are emitted.
   virtual void EmitXXStructor(const DataLayout &DL, const Constant *CV) {
     EmitGlobalConstant(DL, CV);
   }
 

llvm/include/llvm/MC/MCAsmInfo.h

protected:
   // If true, emit GOTPCRELX/REX_GOTPCRELX instead of GOTPCREL, on
   // X86_64 ELF.
   bool RelaxELFRelocations = true;
 
   // If true, then the lexer and expression parser will support %neg(),
   // %hi(), and similar unary operators.
   bool HasMipsExpressions = false;
 
-  // If true, emit function descriptor symbol on AIX.
-  bool NeedsFunctionDescriptors = false;
-
 public:
   explicit MCAsmInfo();
   virtual ~MCAsmInfo();
 
   /// Get the code pointer size in bytes.
   unsigned getCodePointerSize() const { return CodePointerSize; }
 
   /// Get the callee-saved register stack slot
void setCompressDebugSections(DebugCompressionType CompressDebugSections) {
     this->CompressDebugSections = CompressDebugSections;
   }
 
   bool shouldUseLogicalShr() const { return UseLogicalShr; }
 
   bool canRelaxRelocations() const { return RelaxELFRelocations; }
   void setRelaxELFRelocations(bool V) { RelaxELFRelocations = V; }
   bool hasMipsExpressions() const { return HasMipsExpressions; }
-  bool needsFunctionDescriptors() const { return NeedsFunctionDescriptors; }
 };
 
 } // end namespace llvm
 
 #endif // LLVM_MC_MCASMINFO_H

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

void AsmPrinter::EmitFunctionHeader() {
 
   // Print out constants referenced by the function
   EmitConstantPool();
 
   // Print the 'header' of function.
   OutStreamer->SwitchSection(getObjFileLowering().SectionForGlobal(&F, TM));
   EmitVisibility(CurrentFnSym, F.getVisibility());
 
-  if (MAI->needsFunctionDescriptors() &&
+  if (TM.getTargetTriple().isOSAIX() &&
       F.getLinkage() != GlobalValue::InternalLinkage)
     EmitLinkage(&F, CurrentFnDescSym);
 
   EmitLinkage(&F, CurrentFnSym);
   if (MAI->hasFunctionAlignment())
     EmitAlignment(MF->getAlignment(), &F);
 
   if (MAI->hasDotTypeDotSizeDirective())
if (MAI->hasSubsectionsViaSymbols()) {
 
       // Emit an .alt_entry directive for the actual function symbol.
       OutStreamer->EmitSymbolAttribute(CurrentFnSym, MCSA_AltEntry);
     } else {
       EmitGlobalConstant(F.getParent()->getDataLayout(), F.getPrefixData());
     }
   }
 
-  // Emit the function descriptor. This is a virtual function to allow targets
-  // to emit their specific function descriptor.
-  if (MAI->needsFunctionDescriptors())
-    EmitFunctionDescriptor();
-
   // Emit the CurrentFnSym. This is a virtual function to allow targets to do
   // their wild and crazy things as required.
   EmitFunctionEntryLabel();
 
   // If the function had address-taken blocks that got deleted, then we have
   // references to the dangling symbols.  Emit them at the start of the function
   // so that we don't get references to undefined symbols.
   std::vector<MCSymbol*> DeadBlockSyms;
MCSymbol *AsmPrinter::getCurExceptionSym() {
   return CurExceptionSym;
 }
 
 void AsmPrinter::SetupMachineFunction(MachineFunction &MF) {
   this->MF = &MF;
   const Function &F = MF.getFunction();
 
   // Get the function symbol.
-  if (MAI->needsFunctionDescriptors()) {
-    assert(TM.getTargetTriple().isOSAIX() && "Function descriptor is only"
-                                             " supported on AIX.");
+  if (TM.getTargetTriple().isOSAIX()) {
     assert(CurrentFnDescSym && "The function descriptor symbol needs to be"
                                " initalized first.");
 
     // Get the function entry point symbol.
     CurrentFnSym =
         OutContext.getOrCreateSymbol("." + CurrentFnDescSym->getName());
 
     MCSectionXCOFF *FnEntryPointSec =

llvm/lib/MC/MCAsmInfoXCOFF.cpp

 MCAsmInfoXCOFF::MCAsmInfoXCOFF() {
   IsLittleEndian = false;
   HasDotTypeDotSizeDirective = false;
   COMMDirectiveAlignmentIsInBytes = false;
   LCOMMDirectiveAlignmentType = LCOMM::Log2Alignment;
   UseDotAlignForAlignment = true;
   AsciiDirective = nullptr; // not supported
   AscizDirective = nullptr; // not supported
-  NeedsFunctionDescriptors = true;
   HasDotLGloblDirective = true;
   Data64bitsDirective = "\t.llong\t";
   SupportsQuotedNames = false;
 }
 
 bool MCAsmInfoXCOFF::isAcceptableChar(char C) const {
   // QualName is allowed for a MCSymbolXCOFF, and
   // QualName contains '[' and ']'.
   if (C == '[' || C == ']')
     return true;
 
   return MCAsmInfo::isAcceptableChar(C);
 }

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp

public:
   StringRef getPassName() const override { return "AIX PPC Assembly Printer"; }
 
   void SetupMachineFunction(MachineFunction &MF) override;
 
   const MCExpr *lowerConstant(const Constant *CV) override;
 
   void EmitGlobalVariable(const GlobalVariable *GV) override;
 
-  void EmitFunctionDescriptor() override;
+  void EmitFunctionEntryLabel() override;
 
   void EmitEndOfAsmFile(Module &) override;
 };
 
 } // end anonymous namespace
 
 void PPCAsmPrinter::PrintSymbolOperand(const MachineOperand &MO,
                                        raw_ostream &O) {
void PPCAIXAsmPrinter::EmitGlobalVariable(const GlobalVariable *GV) {
 
   MCSymbol *EmittedInitSym = GVSym;
   EmitLinkage(GV, EmittedInitSym);
   EmitAlignment(getGVAlignment(GV, DL), GV);
   OutStreamer->EmitLabel(EmittedInitSym);
   EmitGlobalConstant(GV->getParent()->getDataLayout(), GV->getInitializer());
 }
 
-void PPCAIXAsmPrinter::EmitFunctionDescriptor() {
+void PPCAIXAsmPrinter::EmitFunctionEntryLabel() {
   const DataLayout &DL = getDataLayout();
   const unsigned PointerSize = DL.getPointerSizeInBits() == 64 ? 8 : 4;
 
   MCSectionSubPair Current = OutStreamer->getCurrentSection();
   // Emit function descriptor.
   OutStreamer->SwitchSection(
       cast<MCSymbolXCOFF>(CurrentFnDescSym)->getContainingCsect());
   OutStreamer->EmitLabel(CurrentFnDescSym);
   // Emit function entry point address.
   OutStreamer->EmitValue(MCSymbolRefExpr::create(CurrentFnSym, OutContext),
                          PointerSize);
   // Emit TOC base address.
   const MCSectionXCOFF *TOCBaseSec = OutStreamer->getContext().getXCOFFSection(
       StringRef("TOC"), XCOFF::XMC_TC0, XCOFF::XTY_SD, XCOFF::C_HIDEXT,
       SectionKind::getData());
   const MCSymbol *TOCBaseSym = TOCBaseSec->getQualNameSymbol();
   OutStreamer->EmitValue(MCSymbolRefExpr::create(TOCBaseSym, OutContext),
                          PointerSize);
   // Emit a null environment pointer.
   OutStreamer->EmitIntValue(0, PointerSize);
 
   OutStreamer->SwitchSection(Current.first, Current.second);
+  AsmPrinter::EmitFunctionEntryLabel();
 }
 
 void PPCAIXAsmPrinter::EmitEndOfAsmFile(Module &M) {
   // If there are no functions in this module, we will never need to reference
   // the TOC base.
   if (M.empty())
     return;