[PATCH] D70461: [AIX] Emit TOC entries for ASM printing

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
Comment at: llvm/include/llvm/MC/MCAsmInfo.h:337
+  // This attribute is used to indicate symbols such as Commons on AIX may have
+  // a Storage Mapping Class embedded in the name.
+  bool SymbolsHaveSMC = false;
----------------
nit: Do we need to capitalize Commons and Storage Mapping Class? I find them all in small letters in AIX assembler guide.


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp:33
 #include "llvm/MC/MCSymbolELF.h"
+#include "llvm/MC/MCSymbolXCOFF.h"
 #include "llvm/Support/Casting.h"
----------------
If we intend to keep this interface change:
emitTCEntry(const MCSymbol &, const MCSymbol &)

We could remove this include directive. 


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp:111
 
-  void emitTCEntry(const MCSymbol &S) override {
+  void emitTCEntry(const MCSymbol &Entry, const MCSymbol &Target) override {
+    const MCAsmInfo *MAI = Streamer.getContext().getAsmInfo();
----------------
I'm on the fence of this interface change.
I could see this interface change makes emitTCEntry a bit cleaner, and it models the TC entry a bit better: a symbol represent the real TC, and a symbol represent the item that TC is trying to store. 
But on the other hand, I don't think we are ever getting ".tc A[TC], B", and we always have ".tc A[TC], A" (at least for now). Using the new interface would kinda make that implicit connection lost. 


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:169
+private:
+  void ValidateGV(const GlobalVariable *GV);
+protected:
----------------
Could be a static member function? 


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1910
+
+  return PPCAsmPrinter::getMCSymbolForTOCPseudoMO(MO);
+}
----------------
Could we use this structure for the function? I think it's easier to read and understand. 

```
if (MO.getType() != MachineOperand::MO_GlobalAddress)
  return PPCAsmPrinter::getMCSymbolForTOCPseudoMO(MO);

const GlobalVariable *GV = MO.getGlobal();
ValidateGV(GV)
if (GV->isDeclaration()) {
  if (!Xsym->hasContainingCsect()) {
    // Set up csect for external symbol, use isa<Function> to figure out what SMC we should use for the csect. 
  }
  return Csect->getQualNameSymbol();
}

SectionKind GVKind = getObjFileLowering().getKindForGlobal(GV, TM);
if (GVKind.isCommon() || GVKind.isBSSLocal()) {
  return Csect->getQualNameSymbol();
}

return getSymbol(GV);
```

Some remark on some implication on this change:
1.
```
 !(GO = dyn_cast<const GlobalObject>(MO.getGlobal())  
```
 I'm not sure why we need this dyn_cast on the first if, so I removed it.

2. 

```
const GlobalVariable *GV = dyn_cast<const GlobalVariable>(GO);
  if (GV) {
    ValidateGV(GV);
```
This is a upcast, it's definitely going to succeed. So we don't need dyn_cast, and we will get a GV for sure. 

3. On your current function, it will return a qual name for a defined function. And this structure will not return a qual name for the defined function.
  I think the qual name for the defined function is unnecessary, and use label to refer to that function will succeed. And it's easier on the mental model for when we should use a qual name for TC entry: when the symbol is undefined, or when it's common. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70461