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

[Sean Fertile via Phabricator via llvm-commits]

sfertile added a comment.

In D66724 <https://reviews.llvm.org/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 <https://reviews.llvm.org/D66742> is superior.

In D66724 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72989