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

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
Comment at: llvm/lib/MC/MCExpr.cpp:445
 void MCSymbolRefExpr::printVariantKind(raw_ostream &OS) const {
-  if (UseParensForSymbolVariant)
+  if (getKind() == MCSymbolRefExpr::VK_PPC_TOC_TC0)
+    OS << '[' << MCSymbolRefExpr::getVariantKindName(getKind()) << ']';
----------------
I don't quite like how we do the special case here just for VK_PPC_TOC_TC0 kind.
Not sure if we can do something better. Or we could leave it like this for now, and if we find other MCSymbolRefExpr like this, we could common them up. 
Any thoughts, @sfertile @hubert.reinterpretcast  ?


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1716
+  OutStreamer->EmitLabel(CurrentFnDescSym);
+  OutStreamer->EmitValue(MCSymbolRefExpr::create(CurrentFnSym, OutContext),
+                         PointerSize);
----------------
Minor nit: add a comment before this line
Emit TOC base address.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1734
+  if (M.size() > 0) {
+    StringRef TOC("TOC");
+    MCSymbol *TOCBaseSym = OutContext.getOrCreateSymbol(TOC);
----------------
Minor nit for the naming here:
We seems to use TOC anchor and TOC base to indicate the same thing interchangeably. 
It might be better to use one term through out. In the assembler guide for AIX, we use TOC anchor only. 
However, in llvm source, we use TOC base instead.
So maybe it's better to just be consistent with the existing source, and use the term TOC base?
Some suggestions on the naming:
Rename TOC to TOCStr
Rename TOCAnchor to TOCBaseSec




================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1740
+    cast<MCSymbolXCOFF>(TOCBaseSym)->setContainingCsect(TOCAnchor);
+    // Switch to section to emit TOC-base.
+    OutStreamer->SwitchSection(TOCAnchor);
----------------
Minor nit: emit TOC base.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66724