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

[Digger via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:673
 
+  if (MAI->needsFunctionDescriptors() &&
+      F.getLinkage() != GlobalValue::InternalLinkage)
----------------
if we can use if  (TM.getTargetTriple().isOSAIX()  && here,  we do not need  needsFunctionDescriptors() function and variable needsFunctionDescriptors?


================
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"
----------------
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).


================
Comment at: llvm/lib/MC/MCSectionXCOFF.cpp:20
+  switch (SMC) {
+  case XCOFF::XMC_DS:
+    return "DS";
----------------
since we will have a lot of StorageMappingClass value, can we use a macro like 
#define ECASE(X)  case XCOFF::XMC_##X: return #X
switch (SMC) {
  ECASE(DS);
  ECASE(RW);
}

or we can use array StorageMappingClassArray = {"PR", "RO","DB".......,"TE"};
return StorageMappingClassArray [SMC];

it maybe more simple or readable.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1724
+  // Emit a null environment pointer.
+  OutStreamer->EmitIntValue(0, PointerSize);
+
----------------
from the begin of the function to here, it actually emit a function description, can we add a new function and put the content into the new function and invoke the new function here ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66724