[PATCH] D84765: [AIX][XCOFF] change oprand of branch instruction from symbol name to qualify symbol name.

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
Comment at: llvm/include/llvm/MC/MCSectionXCOFF.h:66
   XCOFF::StorageMappingClass getMappingClass() const { return MappingClass; }
-  XCOFF::StorageClass getStorageClass() const { return StorageClass; }
+  XCOFF::StorageClass getStorageClass() const { return QualName->getStorageClass(); }
   XCOFF::SymbolType getCSectType() const { return Type; }
----------------
How necessary is this convenient function? It might be better to just let people do the call to QualName themselves to avoid confusion, as storage class is not a property of MCSection any more.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1535
     MCSymbol *FnEntryPointSym = TLOF.getFunctionEntryPointSymbol(&F, TM);
-    if (cast<MCSymbolXCOFF>(FnEntryPointSym)->hasRepresentedCsectSet())
-      // Emit linkage for the function entry point.
----------------
After this change, do we still have any call to `hasRepresentedCsectSet`? If not, could we remove that function all together?


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:2167
   // function entry point label any more. We will use function entry
   // point csect instead. For function delcarations, it's okay to continue
   // using label semantic because undefined symbols gets treated as csect with
----------------
comment needs to be update here.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1906
     OutStreamer->emitSymbolAttribute(Sym, MCSA_Extern);
-  return Ret;
+  return PPCAsmPrinter::doFinalization(M);
 }
----------------
There seems to be a switch of execution order here, any reason for that?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5172
+      // C-linkage name and storage mapping classs[PR].
+      const auto getFunctionEntryPointQualNameSymbol = [&](StringRef SymName) {
         auto &Context = DAG.getMachineFunction().getMMI().getContext();
----------------
I don't think it's necessary to change the name, as the sister function in TLOF have the same name but still return qualname when needed.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5176
+        MCSectionXCOFF *Sec =
+            Context.getXCOFFSection(SymQualName.str(), XCOFF::XMC_PR,
+                                    XCOFF::XTY_ER, SectionKind::getMetadata());
----------------
Dont use twine in local. You could do 
```
Context.getXCOFFSection(("." + Twine(SymName)).str(), ....
```


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-toc.ll:138
 ; SYM-NEXT: }
+; SYM-NEXT:   Symbol {
+; SYM-NEXT:     Index: [[#UNDEF_INDX+6]]
----------------
nit: align the spaces.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84765