[PATCH] D78045: [XCOFF][AIX] Fix getSymbol to return the correct qualname when necessary

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:1907
+  // This could change for a GV that is a GlobalVariable when we decide to
+  // support -fdata-sections, since we could avoid having label symbols if the
+  // linkage name is applied to the csect symbol.
----------------
No comma before "since".


================
Comment at: llvm/lib/Target/TargetMachine.cpp:263
+  if (getTargetTriple().isOSBinFormatXCOFF()) {
+    return TLOF->getTargetSymbol(GV, *this);
+  }
----------------
jasonliu wrote:
> **To reviewers**, 
> I'm not sure if this is a preferred way to inject target specific behavior. Or we want to override TargetMachine::getSymbol directly instead.
> Please let me know if you find the other way is more appealing, or you could think of a better way to inject this target specific behavior.
> Thanks.
Can `getTargetSymbol` return `nullptr` to indicate that "falling through" to the generic path is okay? This means that the three lines below don't need to be duplicated.


================
Comment at: llvm/lib/Target/TargetMachine.cpp:261
   const TargetLoweringObjectFile *TLOF = getObjFileLowering();
+  // MCSymbolXCOFF has special naming convention.
+  if (getTargetTriple().isOSBinFormatXCOFF()) {
----------------
Suggestion:
XCOFF symbols have a special naming convention.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-lower-comm.ll:2
+; RUN: llc -verify-machineinstrs -mcpu=pwr4 -mtriple powerpc-ibm-aix-xcoff < %s | FileCheck --check-prefixes=CHECK,ASM32 %s 
+; RUN: llc -verify-machineinstrs -mcpu=pwr4 -mtriple powerpc64-ibm-aix-xcoff < %s | FileCheck --check-prefixes=CHECK,ASM64 %s 
+
----------------
The two lines above have trailing whitespace.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-lower-comm.ll:26
+; RELOC:      Relocations [
+; RELOC-NEXT:   Section (index: 2) .data {
+; RELOC-NEXT:   Relocation {
----------------
I think `{{[0-9]+}}` or similar should be used here. The file does not need to have a `.text` section.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-lower-comm.ll:85
+; SYM-NEXT:     CSECT Auxiliary Entry {
+; SYM-NEXT:       Index: [[#INDX+5]]
+; SYM-NEXT:       SectionLen: 4
----------------
If it is important that there is no entry between `pointer` and `common`, then we can base everything off `INDX`. I don't think we are actually checking for that here, so we should use `COM_INDX+1` in place of `INDX+5`.


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

https://reviews.llvm.org/D78045