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

[Xiangling Liao via Phabricator via llvm-commits]

Xiangling_L marked 20 inline comments as done.
Xiangling_L added inline comments.


================
Comment at: llvm/include/llvm/MC/MCExpr.h:238
     VK_PPC_TOC_HA,         // symbol at toc@ha
+    VK_PPC_TOC_TC0,        // symbol at tc0
     VK_PPC_DTPMOD,         // symbol at dtpmod
----------------
hubert.reinterpretcast wrote:
> The comment is meant to express what the variant looks like when printed (thus, `@` does not match). In any case, I have doubts that this is the correct way to represent `[TC0]`. We can look at `TOC[TC0]` as referring less to the symbol, but more referring to the csect. In any case, the `TC0` is the storage mapping class, which we have been using uppercase for.
Thanks, I will update the comment. But I verified on AIX with Clangtana and GCC, it's TOC[tc0] as TOC base address in function descriptor.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:673
 
+  if (MAI->needsFunctionDescriptors() &&
+      F.getLinkage() != GlobalValue::InternalLinkage)
----------------
DiggerLin wrote:
> if we can use if  (TM.getTargetTriple().isOSAIX()  && here,  we do not need  needsFunctionDescriptors() function and variable needsFunctionDescriptors?
Yes, querying target will lead to correct result as well, but using `MAI->needsFunctionDescriptors()` is the way that we embed target-specific feature to target-independent file `AsmPrinter.cpp` and I think it is more self-explanatory.


================
Comment at: llvm/lib/MC/MCSectionXCOFF.cpp:20
+  switch (SMC) {
+  case XCOFF::XMC_DS:
+    return "DS";
----------------
DiggerLin wrote:
> 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.
I like your idea to make this snippet of code simpler and more readable. But it seems that `StorageMappingClassArray` cannot handle `report_fatal_error` situation. And personally, I feel it's okay to keep it this way by wasting a little bit more space rather than spending time searching and reading macro. 


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1724
+  // Emit a null environment pointer.
+  OutStreamer->EmitIntValue(0, PointerSize);
+
----------------
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.




================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1731
+void PPCAIXAsmPrinter::EmitEndOfAsmFile(Module &M) {
+  // If there are no functions in this module, we will never need to reference
+  // the TOC anchor.
----------------
jasonliu wrote:
> sfertile wrote:
> > There may be symbol references we could emit in assembly that require a TOC-base. Its not really an issue now but it may be once we implement the integrated assembler for AIX. I'm not sure if its better to be `sloppy` and simply always emit a TOC-base (an unused one would be benign ...) 
> I have a slight preference on emitting TOC-base on a needed base, and deal with the issue that integrated assembler introduce when we see one. In this way, we know exactly when we need to emit TOC-base and when not.
> And yes, an unused TOC-base would just get garbage collected by linker; GCC on AIX would also always emit a TOC-base. Hence, it's a slight preference on my side. 
Currently, I prefer leaving it to emit `.toc` when needed as Jason said we may want to know exactly when we need to emit TOC anchor. But I will leave this discussion open and see if there are more input about this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66724