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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1724
+  // Emit a null environment pointer.
+  OutStreamer->EmitIntValue(0, PointerSize);
+
----------------
sfertile wrote:
> Xiangling_L wrote:
> > sfertile wrote:
> > > DiggerLin wrote:
> > > > 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 ?
> > > A good suggestion. I think we should take it a bit farther though. Add an `AsmPrinter::EmitDescriptor()` virtual function. Calling the base class implementation should be a fatal_error. Then somewhere we can explicitly check the MCAsmInfo if descriptors are needed, and call this new virtual if they are. My suggestion would be in `runOnMachineFunction`.
> > Some description of function `EmitFunctionEntryLabel()`:
> > 
> > 
> > > This is a virtual function to allow targets to do their wild and crazy things as required.
> > 
> > So my understanding is on AIX, we use this function to emit function descriptor as the comment specifies.
> > 
> > 
> I think we are pushing past the limits of what the comment intended. I take it to mean 'the wild and crazy function entry point things`. The way the ELF V1 abi handles descriptors (with the entry point being a local symbol, and the descriptor being the global symbol) it kindof squeaks into this definition. For AIX we are emitting 2 distinct symbols, one of which has no relation to the function entry point symbol other then referencing its address.
Reading the generic AsmPrinter code, when `if (MAI->needsFunctionDescriptors())` made me a bit sad.

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.

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.

Please see  D72989.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66724