[PATCH] D78045: [XCOFF][AIX] Fix getSymbol to return the correct qualname when necessary
Hubert Tong via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Apr 14 09:06:20 PDT 2020
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
More information about the llvm-commits
mailing list