[PATCH] D66724: [AIX]Emit function descriptor csect in assembly

[Sean Fertile via Phabricator via llvm-commits]

sfertile added a comment.





================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1657
   // Get the function symbol.
-  CurrentFnSym = getSymbol(&MF.getFunction());
+  if (MAI->needsFunctionDescriptors()) {
+    assert(TM.getTargetTriple().isOSAIX() && "Function descriptor is only"
----------------
DiggerLin wrote:
> sfertile wrote:
> > The code for creating the entry point and descriptor symbols and their csects looks good, I don't think it needs any changes for functionality/correctness. I have a suggestion related to style though. We are inserting about 20 lines of AIX specific implementation details into a constructor thats only about 15 lines long before the change. Most developers that read this won't care about the AIX specific details, should we split the AIX entry point and descriptor setup into some helper functions? Something like
> > 
> > ```
> > if ((MAI->needsFunctionDescriptors()) {
> >    assert(TM.getTargetTriple().isOSAIX() && "Function descriptor is only"
> >                                              " supported on AIX.");
> >   CurrentFnDescSym = getAIXDescriptorSymbol(MF.getFunction(), OutStreamer->getContext());
> >   CurrentFnSym = getAIXEntryPointSymbol(MF.getFunction(),  cast<MCSectionXCOFF>(getObjFileLowering(), TM);
> > } else {
> > ```
> > 
> > keeps this function very readable by having descriptive helper functions bury the implementation details.
> 1. why using "if (MAI->needsFunctionDescriptors()) " here . can we using if(TM.getTargetTriple().isOSAIX) { here. I think for all aix assembly, they all need function description . and then we can delete the needsFunctionDescriptors from the patch ? and we can also delete     assert(TM.getTargetTriple().isOSAIX() && "Function descriptor is only supported on AIX.");
> 2. I have another suggestion is that we make void PPCAIXAsmPrinter::SetupMachineFunction(MachineFunction &MF) as  virtual function(and keep the content), and add new function  AsmPrinter::SetupMachineFunction(MachineFunction &MF), put the new content in the PPCAIXAsmPrinter::SetupMachineFunction(MachineFunction &MF).
>>@DiggerLin wrote:
> why using "if (MAI->needsFunctionDescriptors()) " here . can we using if(TM.getTargetTriple().isOSAIX) { here. I think for all aix assembly, they all need function description . and then we can delete the needsFunctionDescriptors from the patch ?

>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. Personally I slightly prefer the way the patch is doing this already mainly because `needsFunctionDescriptors()` makes the code self-documenting as opposed to `getTargetTriple().isOSAIX()`.


>>@DiggerLin wrote:
> 2. I have another suggestion is that we make void PPCAIXAsmPrinter::SetupMachineFunction(MachineFunction &MF) as virtual function ...

This suggestion of making `SetupMachineFunction` virtual is better then my suggestion of a local helper function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66724