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

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast 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
----------------
sfertile wrote:
> hubert.reinterpretcast wrote:
> > Xiangling_L wrote:
> > > 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.
> > XL generates `TOC{TC0}`, not `TOC[tc0]`.
> According to the assembly language ref:
> ```
> A qualname operand takes the form of:
> symbol[XX]
> OR
> symbol{XX}
> ```
> So both bracketing formats are acceptable, I believe we have been using '[XX]' so far in llvm.
> 
> The assembly language ref also uses both uppercase and lowercase spellings for the storage mapping class, but I believe we have used uppercase exclusively so far in llvm.
Yes, and the group of comments was regarding the capitalization. The comment indicating the bracketing used was a counterexample to the response made to my comment regarding the capitalization.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1742
+                         PointerSize);
+  MCSymbol *TOCBaseSym = OutContext.getOrCreateSymbol(StringRef("TOC"));
+  OutStreamer->EmitValue(
----------------
hubert.reinterpretcast wrote:
> I do not believe that the TOC base is a variant of unrelated symbols coincidentally named "TOC".
Jason and I discussed this off-list, and we believe this to be too much of a hack. In the long term, we probably need some way to separate things that are defined as a label (or QualName without a storage mapping class) from things that are defined by a QualName that has a storage mapping class (like TOC entries). The way a reference should be written in the assembly depends on the form of the definition. One of the decisions to be made is whether they are all `MCSymbol`s or only some of them are `MCSymbol`s. If they are all `MCSymbol`s then we need to differentiate them on more than just the plain name. As far as the system assembler behaviour is concerned, even the name and storage mapping class is not sufficiently unique (it is possible to generate an external reference to a symbol whose name and storage mapping class is the same as another symbol in the assembler source file). A compromise that might be okay for now is to write the csect reference using a symbol named "TOC[TC0]".


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1762
+  StringRef TOC(".toc");
+  MCSymbol *TOCBaseSym = OutContext.getOrCreateSymbol(TOC);
+  MCSectionXCOFF *TOCBaseSection = OutStreamer->getContext().getXCOFFSection(
----------------
sfertile wrote:
> hubert.reinterpretcast wrote:
> > Why are we naming a symbol `.toc` here and not really doing anything with it here or elsewhere in the patch?
> Do we need a TOC symbol to represent the TOC base? Is the TOC base a csect with no contained symbol, or is it similar to the common csects where there is an implicit symbol with the same name contained in the csect? It seems like we really only need the symbol for our internal MC representation, in which case the name doesn't matter outside of being unique and not clashing with anything in the users namespace.
Since this needs a QualName with the storage mapping class for referencing in the assembly, it is more the former (a csect with no contained symbol).

Note that we could choose to use a QualName with the storage mapping class for the `.comm`-produced csects too, but references to the csect need to match the form used on the `.comm` pseudo-op. Since the form used on the `.comm` pseudo-op affects how references look, I think not using the QualName is the right behaviour in terms of being the least surprising.

As for the question of whether we need a symbol to represent the TOC base, I think the question is better asked with regards to how we want to represent references to the TOC base in the MC representation, the mechanism for generating the symbol table entry in the binary emission, and how we will make those references refer to the correct symbol table entry in the binary emission.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66724